diff --git a/src/app/features/issue/issue-service-interface.ts b/src/app/features/issue/issue-service-interface.ts index dfdc1d3bc2..2cd838fece 100644 --- a/src/app/features/issue/issue-service-interface.ts +++ b/src/app/features/issue/issue-service-interface.ts @@ -41,6 +41,16 @@ export interface IssueServiceInterface { // OPTIONAL // -------- + + /** + * Returns the subset of the given imported tasks that are no longer present + * for this user on the provider (deleted, or reassigned away). The caller + * decides whether and how to remove them locally. Implementations MUST NOT act + * on an inconclusive fetch — an outage or lost access must never look like a + * mass removal. + */ + getRemovedRemoteTasks?(tasks: Task[]): Promise; + getMappedAttachments?(issueData: IssueData): TaskAttachment[]; getNewIssuesToAddToBacklog?( diff --git a/src/app/features/issue/issue.service.spec.ts b/src/app/features/issue/issue.service.spec.ts index eb8b05a3fc..60076f341e 100644 --- a/src/app/features/issue/issue.service.spec.ts +++ b/src/app/features/issue/issue.service.spec.ts @@ -76,6 +76,9 @@ describe('IssueService', () => { 'addAndSchedule', 'addSubTaskTo', 'restoreTask', + 'update', + 'remove', + 'removeMultipleTasks', ]); snackServiceSpy = jasmine.createSpyObj('SnackService', ['open']); workContextServiceSpy = jasmine.createSpyObj('WorkContextService', [], { @@ -725,4 +728,121 @@ describe('IssueService', () => { ); }); }); + + describe('refreshIssueTasks - orphaned task auto-removal', () => { + const provider = { id: 'p1', issueProviderKey: 'PLAINSPACE' } as any; + + const mkTask = (overrides: Partial = {}): Task => + ({ + id: `task-${overrides.issueId ?? 'x'}`, + title: 'Imported', + issueId: 'i1', + issueType: 'PLAINSPACE', + issueProviderId: 'p1', + timeSpent: 0, + timeSpentOnDay: {}, + subTaskIds: [], + attachments: [], + tagIds: [], + projectId: 'project-1', + ...overrides, + }) as Task; + + let plainspaceMock: any; + + beforeEach(() => { + plainspaceMock = service.ISSUE_SERVICE_MAP['PLAINSPACE']; + plainspaceMock.getFreshDataForIssueTasks = jasmine + .createSpy('getFreshDataForIssueTasks') + .and.resolveTo([]); + }); + + it('removes a deleted task that has no local content', async () => { + const gone = mkTask({ issueId: 'gone', id: 'task-gone' }); + plainspaceMock.getRemovedRemoteTasks = jasmine + .createSpy('getRemovedRemoteTasks') + .and.resolveTo([gone]); + + await service.refreshIssueTasks([gone], provider); + + expect(taskServiceSpy.remove).toHaveBeenCalledTimes(1); + const removed = taskServiceSpy.remove.calls.mostRecent().args[0]; + expect(removed.id).toBe('task-gone'); + expect(removed.subTasks).toEqual([]); + }); + + it('keeps a deleted task that has tracked time', async () => { + const tracked = mkTask({ + issueId: 'tracked', + id: 'task-tracked', + timeSpent: 60000, + }); + plainspaceMock.getRemovedRemoteTasks = jasmine + .createSpy('getRemovedRemoteTasks') + .and.resolveTo([tracked]); + + await service.refreshIssueTasks([tracked], provider); + + expect(taskServiceSpy.remove).not.toHaveBeenCalled(); + }); + + ( + [ + ['notes', { notes: 'a thought' }], + ['sub-tasks', { subTaskIds: ['sub-1'] }], + ['attachments', { attachments: [{ id: 'a' } as any] }], + ['a repeat config', { repeatCfgId: 'repeat-1' }], + ['a done state', { isDone: true }], + ] as [string, Partial][] + ).forEach(([label, extra]) => { + it(`keeps a deleted task that has local ${label}`, async () => { + const task = mkTask({ issueId: 'kept', id: 'task-kept', ...extra }); + plainspaceMock.getRemovedRemoteTasks = jasmine + .createSpy('getRemovedRemoteTasks') + .and.resolveTo([task]); + + await service.refreshIssueTasks([task], provider); + + expect(taskServiceSpy.remove).not.toHaveBeenCalled(); + }); + }); + + it('does not remove anything when the provider reports no deletions', async () => { + const alive = mkTask({ issueId: 'alive', id: 'task-alive' }); + plainspaceMock.getRemovedRemoteTasks = jasmine + .createSpy('getRemovedRemoteTasks') + .and.resolveTo([]); + + await service.refreshIssueTasks([alive], provider); + + expect(taskServiceSpy.remove).not.toHaveBeenCalled(); + }); + + it('does not throw if detection fails (poll stays alive)', async () => { + const gone = mkTask({ issueId: 'gone', id: 'task-gone' }); + plainspaceMock.getRemovedRemoteTasks = jasmine + .createSpy('getRemovedRemoteTasks') + .and.rejectWith(new Error('network down')); + + await expectAsync(service.refreshIssueTasks([gone], provider)).toBeResolved(); + expect(taskServiceSpy.remove).not.toHaveBeenCalled(); + }); + + it('removes several orphans in ONE bulk op (not N deleteTask dispatches)', async () => { + const a = mkTask({ issueId: 'a', id: 'task-a' }); + const b = mkTask({ issueId: 'b', id: 'task-b' }); + plainspaceMock.getRemovedRemoteTasks = jasmine + .createSpy('getRemovedRemoteTasks') + .and.resolveTo([a, b]); + + await service.refreshIssueTasks([a, b], provider); + + expect(taskServiceSpy.remove).not.toHaveBeenCalled(); + expect(taskServiceSpy.removeMultipleTasks).toHaveBeenCalledTimes(1); + expect(taskServiceSpy.removeMultipleTasks).toHaveBeenCalledWith([ + 'task-a', + 'task-b', + ]); + }); + }); }); diff --git a/src/app/features/issue/issue.service.ts b/src/app/features/issue/issue.service.ts index 1cb5bdecf7..5f37b9f7fa 100644 --- a/src/app/features/issue/issue.service.ts +++ b/src/app/features/issue/issue.service.ts @@ -28,7 +28,7 @@ import { PLAINSPACE_TYPE, } from './issue.const'; import { TaskService } from '../tasks/task.service'; -import { IssueTask, Task, TaskCopy } from '../tasks/task.model'; +import { IssueTask, Task, TaskCopy, TaskWithSubTasks } from '../tasks/task.model'; import { IssueServiceInterface } from './issue-service-interface'; import { JiraCommonInterfacesService } from './providers/jira/jira-common-interfaces.service'; // Trello is now a plugin — no built-in service needed @@ -396,16 +396,18 @@ export class IssueService { labelParams: pollingLabelParams, }); + const service = this._getService(providerKey); + if (!service) { + this._globalProgressBarService.countDown(); + continue; + } + let updates: { task: Task; taskChanges: Partial; issue: IssueData; }[] = []; try { - const service = this._getService(providerKey); - if (!service) { - continue; - } updates = await service.getFreshDataForIssueTasks( tasksIssueIdsByIssueProviderKey[providerKey], ); @@ -449,6 +451,13 @@ export class IssueService { }); } } + + if (service.getRemovedRemoteTasks) { + await this._removeOrphanedRemoteTasks( + service, + tasksIssueIdsByIssueProviderKey[providerKey], + ); + } } for (const taskWithoutIssueId of tasksWithoutIssueId) { @@ -456,6 +465,69 @@ export class IssueService { } } + /** + * Removes local tasks that are gone from the provider (deleted or reassigned + * away), but only when nothing has been invested in them locally. Failures are + * logged and swallowed so a detection hiccup never aborts the rest of the poll. + */ + private async _removeOrphanedRemoteTasks( + service: IssueServiceInterface, + tasks: Task[], + ): Promise { + try { + const orphaned = await service.getRemovedRemoteTasks!(tasks); + const removable = orphaned.filter((task) => !this._hasLocalContent(task)); + if (removable.length === 0) { + return; + } + // The deleteTask effect that mirrors deletions back to the provider no-ops + // here (Plainspace has no deleteIssue adapter) — which is also why removing + // a reassigned task never deletes the still-living remote item. Each device + // polls independently and the delete op is idempotent, so a concurrent + // same-task delete on another device is harmless. Tasks are childless by + // the _hasLocalContent guard, so an empty subTasks list is accurate. + if (removable.length === 1) { + // Single-task path keeps the built-in deleteTask UNDO snackbar. + this._taskService.remove({ + ...removable[0], + subTasks: [], + } as TaskWithSubTasks); + } else { + // A bulk vanish (e.g. many reassigned at once) collapses to ONE deleteTasks + // op instead of N rapid deleteTask dispatches (sync rule #3: one + // reconciliation = one op). The bulk path has no per-task undo snackbar. + this._taskService.removeMultipleTasks(removable.map((task) => task.id)); + } + } catch (e) { + // Never log the raw error object — it may be an HttpErrorResponse whose + // url/body carry the issue id or task content, and the log is exportable. + IssueLog.err( + 'Failed to remove orphaned issue tasks', + e instanceof Error ? e : String(e), + ); + } + } + + /** + * Whether the task holds local work or a completion record that removal would + * lose. Time tracking is the headline case; notes, sub-tasks, attachments and a + * local repeat config all count. `isDone` is kept too: a finished task is a + * record worth preserving, it covers "reassigned away after I completed it", + * and it decouples removal from the server keeping done items in its list. + * Deliberately excluded: dueDay/dueWithTime and tagIds are seeded by the import + * itself, so every imported task has them. + */ + private _hasLocalContent(task: Task): boolean { + return ( + task.timeSpent > 0 || + (task.subTaskIds?.length ?? 0) > 0 || + !!task.notes || + (task.attachments?.length ?? 0) > 0 || + !!task.repeatCfgId || + task.isDone + ); + } + async addTaskFromIssue({ issueDataReduced, issueProviderId, diff --git a/src/app/features/issue/providers/plainspace/plainspace-common-interfaces.service.spec.ts b/src/app/features/issue/providers/plainspace/plainspace-common-interfaces.service.spec.ts index 08e664ccf4..a3cec91875 100644 --- a/src/app/features/issue/providers/plainspace/plainspace-common-interfaces.service.spec.ts +++ b/src/app/features/issue/providers/plainspace/plainspace-common-interfaces.service.spec.ts @@ -182,4 +182,58 @@ describe('PlainspaceCommonInterfacesService', () => { expect(updates[0]?.taskChanges.isDone).toBe(true); }); }); + + describe('getRemovedRemoteTasks (orphan detection by list-diff)', () => { + const setMyTasks = (ids: string[]): void => { + spyOn( + service as unknown as { _getCfgOnce$: (id: string) => unknown }, + '_getCfgOnce$', + ).and.returnValue(of({})); + (api as unknown as { getMyTasks$: unknown }).getMyTasks$ = () => + of(ids.map((id) => ({ ...issue(null), id }))); + }; + + it('returns tasks missing from my task list (deleted or reassigned away)', async () => { + setMyTasks(['kept']); + const tasks = [ + { issueProviderId: 'p1', issueId: 'kept', issueLastUpdated: 0 }, + { issueProviderId: 'p1', issueId: 'gone', issueLastUpdated: 0 }, + ] as Task[]; + const removed = await service.getRemovedRemoteTasks(tasks); + expect(removed.map((t) => t.issueId)).toEqual(['gone']); + }); + + it('keeps tasks still in my list (done tasks stay in the list)', async () => { + setMyTasks(['a', 'b']); + const tasks = [ + { issueProviderId: 'p1', issueId: 'a', issueLastUpdated: 0 }, + { issueProviderId: 'p1', issueId: 'b', issueLastUpdated: 0 }, + ] as Task[]; + expect(await service.getRemovedRemoteTasks(tasks)).toEqual([]); + }); + + // Critical data-loss guard: getMyTasks$ returns [] both on a fail-soft error + // (offline / bad token / wholesale 404) AND when the caller's membership + // scope is empty (removed from the space). An empty list must NOT be read as + // "everything removed" — else losing access wipes every task fleet-wide. + it('never removes anything when my task list is empty', async () => { + setMyTasks([]); + const tasks = [ + { issueProviderId: 'p1', issueId: 'a', issueLastUpdated: 0 }, + { issueProviderId: 'p1', issueId: 'b', issueLastUpdated: 0 }, + ] as Task[]; + expect(await service.getRemovedRemoteTasks(tasks)).toEqual([]); + }); + + // Generalised gate: a NON-empty list that contains none of my tasks (wrong + // space / garbage / total churn) is just as untrustworthy as an empty one. + it('never removes when the list shares no ids with my tasks', async () => { + setMyTasks(['someone-elses-task']); + const tasks = [ + { issueProviderId: 'p1', issueId: 'a', issueLastUpdated: 0 }, + { issueProviderId: 'p1', issueId: 'b', issueLastUpdated: 0 }, + ] as Task[]; + expect(await service.getRemovedRemoteTasks(tasks)).toEqual([]); + }); + }); }); diff --git a/src/app/features/issue/providers/plainspace/plainspace-common-interfaces.service.ts b/src/app/features/issue/providers/plainspace/plainspace-common-interfaces.service.ts index 66fb4c7603..07ec14d54a 100644 --- a/src/app/features/issue/providers/plainspace/plainspace-common-interfaces.service.ts +++ b/src/app/features/issue/providers/plainspace/plainspace-common-interfaces.service.ts @@ -132,6 +132,56 @@ export class PlainspaceCommonInterfacesService extends BaseIssueProviderService< return updates; } + /** + * Detect imported tasks that are no longer mine on Plainspace, for the safe + * auto-removal of orphans. `getMyTasks$` returns every task assigned to me — + * including done ones — so a task missing from that list was either deleted or + * reassigned away from me; both mean "no longer my task" and are removed the + * same way. Done tasks stay in the list, so completing a task never removes it. + * + * Safety gate against fleet-wide data loss: skip when NONE of my polled tasks + * are still in the fetched list. `getMyTasks$` fails soft to `[]` (offline / + * bad token / wholesale 404), the server returns `[]` when my membership scope + * is empty (removed from the space), and a wrong/garbage list shares no ids + * with mine — reading any of those as "everything was removed" would wipe every + * task on every synced device. A list that still contains some of my tasks + * proves it is live and authorized, so the rest can be trusted as removed. + * Trade-off: if every one of my tasks disappears in one window we skip, leaving + * the orphans until the next appears — the safe direction for a destructive + * action. NB: this trusts the list to be COMPLETE; GET /tasks is unpaginated + * today, so if it ever paginates/truncates the missing entries would be misread + * as removals here. + */ + async getRemovedRemoteTasks(tasks: Task[]): Promise { + const tasksByProviderId = new Map(); + for (const task of tasks) { + if (!task.issueProviderId || !task.issueId) { + continue; + } + const group = tasksByProviderId.get(task.issueProviderId) ?? []; + group.push(task); + tasksByProviderId.set(task.issueProviderId, group); + } + + const removed: Task[] = []; + for (const [providerId, providerTasks] of tasksByProviderId) { + const cfg = await firstValueFrom(this._getCfgOnce$(providerId)); + const myTaskIds = new Set( + (await firstValueFrom(this._plainspaceApiService.getMyTasks$(cfg))).map( + (issue) => issue.id, + ), + ); + const gone = providerTasks.filter((task) => !myTaskIds.has(task.issueId!)); + // All of my polled tasks gone at once → distrust the list, skip (see gate + // above). This subsumes the empty-list case. + if (gone.length === providerTasks.length) { + continue; + } + removed.push(...gone); + } + return removed; + } + private _toFreshData( task: Task, issue: PlainspaceIssue | null,