From bcdee58b6d273cc201cda35e2a8fa97075579ce8 Mon Sep 17 00:00:00 2001 From: trtshen Date: Mon, 11 Aug 2025 16:44:09 +0800 Subject: [PATCH 1/3] [CORE-6673] make sure orphaned indicators get cleared --- docs/unlock-indicator.md | 392 ++++++++++++++++++ .../components/activity/activity.component.ts | 3 +- .../src/app/components/img/img.component.ts | 4 +- .../activity-desktop/activity-desktop.page.ts | 83 +++- .../activity-mobile/activity-mobile.page.ts | 44 +- projects/v3/src/app/pages/home/home.page.ts | 91 +++- .../src/app/services/notifications.service.ts | 37 ++ .../app/services/unlock-indicator.service.ts | 260 +++++++++++- 8 files changed, 876 insertions(+), 38 deletions(-) create mode 100644 docs/unlock-indicator.md diff --git a/docs/unlock-indicator.md b/docs/unlock-indicator.md new file mode 100644 index 000000000..8a391435a --- /dev/null +++ b/docs/unlock-indicator.md @@ -0,0 +1,392 @@ +# Unlock Indicator: Implementation and Integration + +This document explains how the “unlock indicator” (red dot) is implemented, how it’s stored and updated, and how it appears in the UI across the Home page and list items. + +## Files involved +- Service: `projects/v3/src/app/services/unlock-indicator.service.ts` +- Home page (TS): `projects/v3/src/app/pages/home/home.page.ts` +- Home page (HTML): `projects/v3/src/app/pages/home/home.page.html` +- List item component (HTML): `projects/v3/src/app/components/list-item/list-item.component.html` + +## Concept overview +The unlock indicator shows a red dot next to activities that have been “unlocked” by some trigger (e.g., a milestone or task completion). These indicators are persisted in browser storage and exposed via an RxJS observable so that UI can reactively render the dots. + +## Data model and storage +- Interface: `UnlockedTask` + - Fields: `milestoneId?`, `activityId?`, `taskId?`, plus optional metadata. +- State: A `BehaviorSubject` holds all unlocked items. +- Persistence: Items are saved to `BrowserStorageService` under the key `unlockedTasks` and rehydrated in the service constructor. + +### Key service members +- `unlockedTasks$`: Observable emitting the current list of unlocked entries. +- `unlockTasks(data: UnlockedTask[])`: Merges new unlocks with existing ones and de-duplicates by `(milestoneId, activityId, taskId)` combination. +- `clearAllTasks()`: Clears all unlock indicators (e.g., on experience switch). +- `clearActivity(id: number)`: Removes all entries where `activityId === id` OR `milestoneId === id`. Returns the removed entries so callers can mark any related notifications/todos as done. Note: Name is overloaded — it clears by activity or milestone id. +- `clearByActivityId(activityId: number)`: Explicit activity-only clearing (enhanced version). +- `clearByMilestoneId(milestoneId: number)`: Explicit milestone-only clearing (enhanced version). +- `clearByActivityIdWithDuplicates()`: Enhanced clearing that handles server-side TodoItem duplicates and auto-cascades to parent milestones. +- `clearByMilestoneIdWithDuplicates()`: Enhanced milestone clearing with duplicate detection. +- `findDuplicateTodoItems()`: Detects multiple TodoItems for the same logical unlock (handles server-side duplicates). +- `cleanupOrphanedIndicators()`: Removes stale localStorage entries that no longer exist in current API response. +- `removeTasks(taskId?: number)`: Cascading removal when a specific task is visited; if the last task in an activity is removed, it clears that activity; if the last activity/task in a milestone is removed, it clears that milestone as well. +- `isActivityClearable(activityId: number)`: Returns `true` only if there are no remaining task-level unlocks (`taskId`) under that activity. +- `isMilestoneClearable(milestoneId: number)`: Returns `true` only if there are no remaining activity- or task-level unlocks under that milestone. +- `getTasksByActivity(activity: Activity)`, `getTasksByActivityId(id)`, `getTasksByMilestoneId(id)`: Query helpers. + +## Hierarchical Clearing Rules and Duplicate Handling + +### Overview +The unlock indicator system implements a strict hierarchy that prevents premature clearing of parent indicators. Indicators are only clearable when all their children have been cleared, ensuring accurate representation of remaining unlocked content. + +### Hierarchy Structure +``` +Milestone (top-level) +├── Activity (mid-level) +│ ├── Task (leaf-level) +│ └── Task (leaf-level) +└── Activity (mid-level) + └── Task (leaf-level) +``` + +### Clearability Rules +1. **Task-level indicators**: Always clearable when the task is visited/completed +2. **Activity-level indicators**: Only clearable when NO task-level children remain (`isActivityClearable()`) +3. **Milestone-level indicators**: Only clearable when NO activity-level OR task-level children remain (`isMilestoneClearable()`) + +### Clearability Logic +```typescript +isActivityClearable(activityId: number): boolean { + const activities = this.getTasksByActivityId(activityId); + const hasUnlockedTasks = activities.some(task => task.taskId !== undefined); + return !hasUnlockedTasks; // Only clearable if no task-level unlocks remain +} + +isMilestoneClearable(milestoneId: number): boolean { + const milestones = this.getTasksByMilestoneId(milestoneId); + const hasUnlockedActivities = milestones.some(task => task.activityId !== undefined); + const hasUnlockedTasks = milestones.some(task => task.taskId !== undefined); + return !hasUnlockedActivities && !hasUnlockedTasks; // Only clearable if no children remain +} +``` + +### Server-Side Duplicate Problem +The server sometimes creates multiple TodoItem records for the same logical unlock, causing persistent red dots even after partial clearing. + +**Example Problem**: +```json +// localStorage has: +[{"id":25473,"identifier":"NewItem-17432","milestoneId":11212}] + +// API response contains duplicates: +[ + {"id":25473,"identifier":"NewItem-17432","is_done":false}, + {"id":25475,"identifier":"NewItem-17432","is_done":true}, // Already marked + {"id":25474,"identifier":"NewItem-17432","is_done":false} // Still active! +] + +// Problem: Marking only 25473 leaves 25474 active → red dot persists +``` + +### Enhanced Duplicate Detection +```typescript +findDuplicateTodoItems(currentTodoItems, unlockedTask) { + return currentTodoItems.filter(item => { + // Exact identifier match + if (item.identifier === unlockedTask.identifier) return true; + + // Base identifier pattern matching (handles variations) + const baseIdentifier = unlockedTask.identifier.replace(/-\d+$/, ''); + const itemBaseIdentifier = item.identifier.replace(/-\d+$/, ''); + if (itemBaseIdentifier === baseIdentifier) return true; + + // Prefix matching for same unlock event + if (item.identifier.startsWith(baseIdentifier)) return true; + + return false; + }); +} +``` + +### Cascade Clearing Logic +When an activity is cleared, the system automatically checks if parent milestones become clearable: + +```typescript +clearByActivityIdWithDuplicates(activityId, currentTodoItems) { + // 1. Clear activity and find all duplicates + const activityResult = this.clearActivity(activityId); + const duplicates = this.findAllDuplicates(activityResult); + + // 2. Check affected parent milestones + const affectedMilestones = new Set(activityResult.map(t => t.milestoneId)); + const cascadeMilestones = []; + + affectedMilestones.forEach(milestoneId => { + if (this.isMilestoneClearable(milestoneId)) { + // 3. Auto-clear parent milestone if it becomes clearable + const milestoneResult = this.clearByMilestoneIdWithDuplicates(milestoneId, currentTodoItems); + cascadeMilestones.push(milestoneResult); + } + }); + + return { duplicates, cascadeMilestones }; +} +``` + +### Real-World Example +**Initial State**: +```json +localStorage: [ + {"id":25473,"identifier":"NewItem-17432","milestoneId":11212}, // Milestone-level + {"id":25480,"identifier":"NewItem-17434","activityId":26686,"milestoneId":11212} // Activity-level +] + +// Milestone 11212 is NOT clearable (has activity child 26686) +// Activity 26686 IS clearable (no task children) +``` + +**When user visits activity 26686**: +1. **Activity Clearing**: + - Finds duplicates: `[25480, 25479]` for "NewItem-17434" + - Marks both as done via bulk API calls + - Removes activity entry from localStorage + +2. **Cascade Check**: + - Checks: `isMilestoneClearable(11212)` → now `true` (no more children) + - Auto-triggers milestone clearing + +3. **Milestone Clearing**: + - Finds duplicates: `[25473, 25475, 25474]` for "NewItem-17432" + - Marks all as done via bulk API calls + - Removes milestone entry from localStorage + +4. **Final Result**: + - All red dots cleared + - Complete hierarchy resolved + - 5 total API calls (all duplicates marked) + +### Integration with NotificationsService +```typescript +// Enhanced bulk marking capability +markMultipleTodoItemsAsDone(items: {id: number, identifier: string}[]) { + const markingOperations = items.map(item => this.markTodoItemAsDone(item)); + return markingOperations; // Returns array of Observables for parallel execution +} + +// Automatic orphan cleanup during TodoItem fetching +getTodoItems() { + return this.request.get(api.get.todoItem).pipe( + map(response => { + // Clean up stale localStorage entries before processing + this.unlockIndicatorService.cleanupOrphanedIndicators(response.data); + + const normalised = this._normaliseTodoItems(response.data); + return normalised; + }) + ); +} +``` +File: `home.page.ts` + +### Subscription to unlocked tasks (reactive mapping to UI) +On init, the Home page subscribes to `unlockedTasks$` and builds a map `hasUnlockedTasks: { [activityId: number]: true }` used by the template to render red dots. It also proactively clears milestone-level indicators that are now clearable. + +Relevant code excerpt: +- `home.page.ts` — subscription to `unlockedTasks$`: + +``` +this.unlockIndicatorService.unlockedTasks$ + .pipe(distinctUntilChanged(), takeUntil(this.unsubscribe$)) + .subscribe({ + next: (unlockedTasks) => { + this.hasUnlockedTasks = {}; // reset + unlockedTasks.forEach((task) => { + if (task.milestoneId) { + if (this.unlockIndicatorService.isMilestoneClearable(task.milestoneId)) { + this.verifyUnlockedMilestoneValidity(task.milestoneId); + } + } + if (task.activityId) { + this.hasUnlockedTasks[task.activityId] = true; + } + }); + }, + }); +``` + +Notes: +- The mapping sets `hasUnlockedTasks[activityId] = true` for any entry that includes an `activityId`. +- If a milestone is clearable (no remaining child unlocks), `verifyUnlockedMilestoneValidity` is called to clear it and mark related todos as done. + +### Rendering the red dot in the template +- `home.page.html` binds the computed map into each `app-list-item`: + +``` +[redDot]="hasUnlockedTasks[activity.id] || false" +``` + +This is the only flag the list item needs to display the red dot. + +### Clearing indicators when navigating to an activity +The method `gotoActivity({ activity, milestone })` (around lines ~140–161 of `home.page.ts`) includes the clearing logic: +- If the activity is clearable (`isActivityClearable(activity.id)` is `true`), it calls `clearActivity(activity.id)` to remove unlock entries at the activity level, and for each removed entry, calls `NotificationsService.markTodoItemAsDone(...)`. +- Separately, if the milestone is clearable, it calls `verifyUnlockedMilestoneValidity(milestone.id)`, which internally uses `clearActivity(milestoneId)` to clear milestone-level unlocks and mark them as done. + +This ensures the UI and persisted state remain in sync after the user visits relevant activities. + +## List item integration (UI) +File: `list-item.component.html` + +The component renders a red notification dot whenever its `redDot` input is `true`. The dot is shown both for avatar (when `leadImage` is present) and for the default icon container. + +Key snippets: + +``` + +``` + +- When there is a `leadImage`: + - The dot is inside the `ion-avatar` element. +- When there is no `leadImage`: + - The dot is inside the fallback `.icon-container`. + +No additional logic is required in the list item; it purely reflects the `redDot` input. + +## Additional integration points + +- `experiences.page.ts` + - On program switch, calls `unlockIndicatorService.clearAllTasks()` to reset all indicators when changing experiences. + +- `v3.page.ts` + - Subscribes to `unlockIndicatorService.unlockedTasks$` at the app shell level to keep higher-level UI (e.g., sidebar/menu badges or booleans like `hasUnlockedTasks`) in sync with unlock state. + +- `activity.service.ts` + - In `goToTask`, calls `unlockIndicatorService.removeTasks(task.id)` so visiting a specific task clears its task-level indicator and, via cascading logic, clears related activity/milestone indicators when appropriate. Removed items are then marked as done via the notifications flow. + +- `activity.component.ts` + - Subscribes to `unlockIndicatorService.unlockedTasks$` and builds a `newTasks` map keyed by `taskId` to flag per-task “new/unlocked” state inside the activity view. + +- `notifications.service.ts` + - Acts as the source/sink for unlock-related TodoItems. It coordinates marking items as done when cleared (e.g., called from Home/Activity), and standardizes unlock entries. It integrates with `UnlockIndicatorService` (imported) to participate in the unlock pipeline. + +- `auth.service.ts` + - Imports `UnlockIndicatorService`. While program switching reset is handled in `experiences.page.ts`, auth-related flows keep the service available for clearing/reset as needed alongside broader cache clears. + +## End-to-end flow +1. Some part of the app determines new content is unlocked and calls `unlockIndicatorService.unlockTasks([...])` with `UnlockedTask` entries (often originating from normalized TodoItems in the notifications pipeline). +2. Service merges, de-duplicates, persists, and emits the new list via `unlockedTasks$`. +3. Home page subscription rebuilds `hasUnlockedTasks` and clears any now-clearable milestones. UI updates reactively, and the app shell may also react via its own subscription. +4. The Home page template binds `hasUnlockedTasks[activity.id]` to `[redDot]`, so affected activities display a red dot. +5. When the user opens an activity or task: + - **Activity**: if no remaining task-level unlocks exist under that activity (`isActivityClearable` returns `true`), enhanced clearing finds ALL server-side duplicates and marks them as done in parallel. If parent milestone becomes clearable, it auto-cascades to clear milestone duplicates as well. + - **Task**: `ActivityService.goToTask(...)` clears the task-level indicator using `removeTasks(task.id)`, cascading as needed up to activity/milestone. + - **Hierarchy validation**: Each clearing operation respects the hierarchy - milestones only clear when no children remain. +6. The service emits the updated list; the red dot disappears for cleared activities/milestones, and per-task flags update inside the activity view. +7. **Automatic cleanup**: On each TodoItem API fetch, orphaned localStorage entries (no longer in API) are automatically removed. + +## Troubleshooting and Common Issues + +### Symptoms of Problems +- Red dot persists on Home after visiting an activity or a task +- Milestone-level indicator does not clear after all child items are visited +- Dot clears only when entering from Home, but not when deep-linking to a task +- Indicators persist even when no corresponding TodoItems exist in API response + +### Root Causes and Solutions + +#### 1. Entry Path Bypassing +**Problem**: Users enter activities via paths that bypass cleanup logic (deep links, direct task opens). + +**Solution**: Activity pages should include page-enter cleanup logic: +```typescript +// In activity desktop/mobile pages (ionViewDidEnter) +private _clearPureActivityIndicator() { + const activityLevelEntries = this.unlockIndicatorService.getTasksByActivityId(this.activity.id) + .filter(task => task.taskId === undefined); // Only pure activity entries + + if (activityLevelEntries.length > 0 && this.unlockIndicatorService.isActivityClearable(this.activity.id)) { + const result = this.unlockIndicatorService.clearByActivityIdWithDuplicates(this.activity.id, this.currentTodoItems); + // Mark duplicates as done via bulk API calls + } +} +``` + +#### 2. Overloaded Method Confusion +**Problem**: `clearActivity(id)` removes by activityId OR milestoneId, causing ID collision issues. + +**Solution**: Use explicit methods: +- Replace `clearActivity(activityId)` with `clearByActivityId(activityId)` +- Replace `clearActivity(milestoneId)` with `clearByMilestoneId(milestoneId)` + +#### 3. Missing ActivityId in Task Entries +**Problem**: Task-level entries without `activityId` can't be mapped by Home page. + +**Solution**: Enforce `activityId` presence in `NotificationsService._normaliseUnlockedTasks()`: +```typescript +// Ensure task entries always include activityId +if (entry.taskId && !entry.activityId) { + // Derive or skip entry if activityId cannot be determined +} +``` + +#### 4. Orphaned Data and Server Duplicates +**Problem**: Multiple TodoItems created for same unlock, partial clearing leaves active duplicates. + +**Solution**: Enhanced clearing with duplicate detection (already implemented): +- `findDuplicateTodoItems()` identifies all server-side duplicates +- `markMultipleTodoItemsAsDone()` handles bulk API marking +- `cleanupOrphanedIndicators()` removes stale localStorage entries + +### Implementation Checklist for Robustness + +- [ ] **Activity Pages**: Add page-enter cleanup for activity-level-only entries + - Desktop: `projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts` + - Mobile: Equivalent activity page files +- [ ] **Service Methods**: Replace ambiguous `clearActivity` with explicit methods + - `clearByActivityId(activityId: number)` + - `clearByMilestoneId(milestoneId: number)` +- [ ] **Data Validation**: Enforce `activityId` presence for task entries + - File: `projects/v3/src/app/services/notifications.service.ts` +- [ ] **Route Guards**: Optional resolver-based cleanup on activity routes +- [ ] **Testing**: Unit tests for new methods and e2e tests for deep links + +### Debug and Diagnostics + +#### Console Debugging +Enhanced methods provide detailed console output: +``` +"Found X duplicate TodoItems for unlock:" +"Bulk marking X TodoItems as done:" +"Marked duplicate TodoItem as done:" +"Auto-cascading to clear parent milestone:" +``` + +#### Manual Inspection +- **localStorage**: Check `unlockedTasks` key in browser dev tools +- **Router Events**: Log navigation paths to identify bypassed cleanup +- **Service State**: Verify `unlockedTasks$` observable content matches expectations + +#### Test Matrix +1. **Home → Activity → Task**: Verify proper clearing sequence +2. **Direct Activity Entry**: Test page-enter cleanup for activity-only entries +3. **Deep Link to Task**: Ensure task and parent clearing works +4. **Milestone Clearing**: Verify cascade clearing when all children visited +5. **Experience Switch**: Confirm `clearAllTasks()` resets all state + +### Performance Considerations +- **Bulk Operations**: Parallel TodoItem marking reduces API overhead +- **Automatic Cleanup**: Orphaned data removal prevents localStorage bloat +- **Cascade Logic**: Smart parent clearing reduces manual intervention +- **Pattern Matching**: Efficient duplicate detection with regex patterns + +## Edge cases and notes +- **Hierarchy enforcement**: Activity-level clearing is intentionally conservative - it only happens when there are no task-level unlocks (`isActivityClearable` returns `true`). If any task under the activity remains unlocked, the red dot persists. +- **Milestone clearability**: Milestone indicators are NOT manually clearable - they only clear when all their children (activities and tasks) have been cleared. +- **Duplicate handling**: The enhanced system detects and marks ALL server-side TodoItem duplicates, not just the first one found. This prevents persistent red dots caused by partial clearing. +- **Cascade clearing**: When an activity clears, the system automatically checks if its parent milestone should also clear, eliminating the need for manual milestone clearing in most cases. +- **Orphan cleanup**: Stale localStorage entries that no longer exist in the current TodoItem API response are automatically removed during each API fetch. +- `clearActivity(id)` is deprecated in favor of explicit `clearByActivityId()` and `clearByMilestoneId()` methods to avoid ID collision issues. +- For task-level events, prefer `removeTasks(taskId)` to leverage the cascading removal logic (task → activity → milestone) when appropriate. +- When creating `UnlockedTask` entries for tasks, ensure `activityId` is included if the activity-level red dot should be shown for that task; otherwise the Home page cannot map it to an activity and no dot will render. +- The service rehydrates from storage on construction, so indicators persist across reloads. +- **Console debugging**: Enhanced methods provide detailed console output showing duplicate detection, bulk marking operations, and cascade clearing for troubleshooting. +- **Conservative clearing rules**: The intentionally conservative clearing behavior prevents premature dot removal. Ensure product acceptance aligns with these rules before making them more aggressive. diff --git a/projects/v3/src/app/components/activity/activity.component.ts b/projects/v3/src/app/components/activity/activity.component.ts index b5ec0fb3e..cb20ebe1a 100644 --- a/projects/v3/src/app/components/activity/activity.component.ts +++ b/projects/v3/src/app/components/activity/activity.component.ts @@ -116,7 +116,8 @@ export class ActivityComponent implements OnInit, OnChanges, OnDestroy { const unlockedTasks = this.unlockIndicatorService.getTasksByActivity(this.activity); this.resetTaskIndicator(unlockedTasks); if (unlockedTasks.length === 0) { - const clearedActivities = this.unlockIndicatorService.clearActivity(this.activity.id); + // handle inaccurate unlock indicators + const clearedActivities = this.unlockIndicatorService.clearRelatedIndicators('activity', this.activity.id); clearedActivities.forEach((activity) => { this.notificationsService .markTodoItemAsDone(activity) diff --git a/projects/v3/src/app/components/img/img.component.ts b/projects/v3/src/app/components/img/img.component.ts index e6bc39bba..e4ebc8149 100644 --- a/projects/v3/src/app/components/img/img.component.ts +++ b/projects/v3/src/app/components/img/img.component.ts @@ -1,4 +1,4 @@ -import { Component, Input, isDevMode, SimpleChanges } from '@angular/core'; +import { Component, Input, isDevMode, OnChanges, SimpleChanges } from '@angular/core'; import { getData, getAllTags } from 'exif-js'; const getImageClassToFixOrientation = (orientation) => { @@ -32,7 +32,7 @@ const swapWidthAndHeight = img => { templateUrl: './img.component.html', styleUrls: ['./img.component.scss'] }) -export class ImgComponent { +export class ImgComponent implements OnChanges { @Input() alt: string; @Input() imgSrc: string; proxiedImgSrc: string; diff --git a/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts b/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts index 9407be7e4..35ff0bd91 100644 --- a/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts +++ b/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts @@ -302,10 +302,81 @@ export class ActivityDesktopPage { } }); } + // Clear pure activity-level unlock indicators on page enter/update + this._clearPureActivityIndicator(res.id); return; } this.activity = res; + // Clear pure activity-level unlock indicators on initial set + this._clearPureActivityIndicator(res.id); + } + + /** + * clears activity-level unlock indicators on page enter + */ + private _clearPureActivityIndicator(activityId: number): void { + if (!activityId) { return; } + + try { + // First try the enhanced approach that handles duplicates + const currentTodoItems = this.notificationsService.getCurrentTodoItems(); + const entries = this.unlockIndicatorService.getTasksByActivityId(activityId); + + if (entries?.length > 0 && entries.every(e => e.taskId === undefined)) { + // handles server-side duplicates and hierarchy + const result = this.unlockIndicatorService.clearByActivityIdWithDuplicates(activityId, currentTodoItems); + + // Mark all duplicate TodoItems as done (bulk operation) + if (result.duplicatesToMark.length > 0) { + const markingOps = this.notificationsService.markMultipleTodoItemsAsDone(result.duplicatesToMark); + markingOps.forEach(op => op.subscribe({ + // eslint-disable-next-line no-console + next: (response) => console.log('Marked duplicate activity TodoItem as done:', response), + // eslint-disable-next-line no-console + error: (error) => console.error('Failed to mark activity TodoItem as done:', error) + })); + } + + // handles cascade milestone clearing + result.cascadeMilestones.forEach(milestoneData => { + if (milestoneData.duplicatesToMark.length > 0) { + // eslint-disable-next-line no-console + console.log(`Cascade clearing milestone ${milestoneData.milestoneId} with ${milestoneData.duplicatesToMark.length} duplicates`); + const milestoneMarkingOps = this.notificationsService.markMultipleTodoItemsAsDone(milestoneData.duplicatesToMark); + milestoneMarkingOps.forEach(op => op.subscribe({ + // eslint-disable-next-line no-console + next: (response) => console.log('Marked cascade milestone TodoItem as done:', response), + // eslint-disable-next-line no-console + error: (error) => console.error('Failed to mark cascade milestone TodoItem as done:', error) + })); + } + }); + + // Fallback: mark cleared localStorage items as done (for backward compatibility) + result.clearedUnlocks?.forEach(todo => { + this.notificationsService.markTodoItemAsDone(todo).subscribe(); + }); + return; + } + + // If standard approach didn't find anything, try robust clearing for inaccurate data + const relatedIndicators = this.unlockIndicatorService.findRelatedIndicators('activity', activityId); + if (relatedIndicators?.length > 0) { + // Only clear if they are pure activity-level (no task-specific entries) + const pureActivityIndicators = relatedIndicators.filter(r => r.taskId === undefined); + if (pureActivityIndicators.length > 0) { + const cleared = this.unlockIndicatorService.clearRelatedIndicators('activity', activityId); + cleared?.forEach(todo => { + this.notificationsService.markTodoItemAsDone(todo).subscribe(); + }); + } + } + } catch (e) { + // swallow to avoid breaking page enter; optional logging can be added under dev flag + // eslint-disable-next-line no-console + console.debug('[unlock-indicator] cleanup skipped for activity', activityId, e); + } } /** @@ -396,25 +467,25 @@ export class ActivityDesktopPage { try { // handle unexpected submission: do final status check before saving let hasSubmssion = false; - const { submission } = await this.assessmentService - .fetchAssessment( + const { submission } = await firstValueFrom( + this.assessmentService.fetchAssessment( event.assessmentId, 'assessment', this.activity.id, event.contextId, event.submissionId ) - .toPromise(); + ); if (submission?.status === 'in progress') { - const saved = await this.assessmentService - .submitAssessment( + const saved = await firstValueFrom( + this.assessmentService.submitAssessment( event.submissionId, event.assessmentId, event.contextId, event.answers ) - .toPromise(); + ); // http 200 but error if ( diff --git a/projects/v3/src/app/pages/activity-mobile/activity-mobile.page.ts b/projects/v3/src/app/pages/activity-mobile/activity-mobile.page.ts index b2fd03535..b9f2eb959 100644 --- a/projects/v3/src/app/pages/activity-mobile/activity-mobile.page.ts +++ b/projects/v3/src/app/pages/activity-mobile/activity-mobile.page.ts @@ -3,6 +3,8 @@ import { ActivatedRoute, Router } from '@angular/router'; import { ActivityService, Task, Activity } from '@v3/services/activity.service'; import { AssessmentService, Submission } from '@v3/services/assessment.service'; import { filter } from 'rxjs/operators'; +import { UnlockIndicatorService } from '@v3/app/services/unlock-indicator.service'; +import { NotificationsService } from '@v3/app/services/notifications.service'; @Component({ selector: 'app-activity-mobile', @@ -18,18 +20,58 @@ export class ActivityMobilePage implements OnInit { private router: Router, private activityService: ActivityService, private assessmentService: AssessmentService, + private unlockIndicatorService: UnlockIndicatorService, + private notificationsService: NotificationsService, ) { } ngOnInit() { this.activityService.activity$ .pipe(filter(res => res?.id === +this.route.snapshot.paramMap.get('id'))) - .subscribe(res => this.activity = res); + .subscribe(res => { + this.activity = res; + if (res?.id) { + this.clearPureActivityIndicator(res.id); + } + }); this.assessmentService.submission$.subscribe(res => this.submission = res); this.route.params.subscribe(params => { this.activityService.getActivity(+params.id, false); }); } + /** + * Clear activity-level-only unlock indicators when entering the activity page. + * Uses robust clearing to handle inaccurate unlock indicator data. + */ + private clearPureActivityIndicator(activityId: number) { + if (!activityId) { return; } + + try { + // First try the standard approach + const entries = this.unlockIndicatorService.getTasksByActivityId(activityId); + if (entries?.length > 0 && entries.every(e => e.taskId === undefined)) { + const cleared = this.unlockIndicatorService.clearByActivityId(activityId); + cleared?.forEach(todo => this.notificationsService.markTodoItemAsDone(todo).subscribe()); + return; + } + + // If standard approach didn't find anything, try robust clearing for inaccurate data + const relatedIndicators = this.unlockIndicatorService.findRelatedIndicators('activity', activityId); + if (relatedIndicators?.length > 0) { + // Only clear if they are pure activity-level (no task-specific entries) + const pureActivityIndicators = relatedIndicators.filter(r => r.taskId === undefined); + if (pureActivityIndicators.length > 0) { + const cleared = this.unlockIndicatorService.clearRelatedIndicators('activity', activityId); + cleared?.forEach(todo => this.notificationsService.markTodoItemAsDone(todo).subscribe()); + } + } + } catch (e) { + // swallow to avoid breaking page enter; optional logging can be added under dev flag + // eslint-disable-next-line no-console + console.debug('[unlock-indicator] cleanup skipped for activity', activityId, e); + } + } + goToTask(task: Task) { this.activityService.goToTask(task, false); switch (task.type) { diff --git a/projects/v3/src/app/pages/home/home.page.ts b/projects/v3/src/app/pages/home/home.page.ts index 00738fdf2..88b2c773e 100644 --- a/projects/v3/src/app/pages/home/home.page.ts +++ b/projects/v3/src/app/pages/home/home.page.ts @@ -287,18 +287,49 @@ export class HomePage implements OnInit, OnDestroy, AfterViewChecked { } if (this.unlockIndicatorService.isActivityClearable(activity.id)) { - const clearedActivityTodo = this.unlockIndicatorService.clearActivity( - activity.id - ); - clearedActivityTodo?.forEach((todo) => { - this.notification - .markTodoItemAsDone(todo) - .pipe(first()) - .subscribe(() => { + // handles server-side duplicates and hierarchy + const currentTodoItems = this.notification.getCurrentTodoItems(); + const result = this.unlockIndicatorService.clearByActivityIdWithDuplicates(activity.id, currentTodoItems); + + // Mark all duplicate TodoItems as done (bulk operation) + if (result.duplicatesToMark.length > 0) { + const markingOps = this.notification.markMultipleTodoItemsAsDone(result.duplicatesToMark); + markingOps.forEach(op => op.pipe(first()).subscribe({ + // eslint-disable-next-line no-console + next: (response) => console.log('Marked duplicate activity TodoItem as done:', response), + // eslint-disable-next-line no-console + error: (error) => console.error('Failed to mark activity TodoItem as done:', error) + })); + } + + // Handle cascade milestone clearing + result.cascadeMilestones.forEach(milestoneData => { + if (milestoneData.duplicatesToMark.length > 0) { + // eslint-disable-next-line no-console + console.log(`Cascade clearing milestone ${milestoneData.milestoneId} with ${milestoneData.duplicatesToMark.length} duplicates`); + const milestoneMarkingOps = this.notification.markMultipleTodoItemsAsDone(milestoneData.duplicatesToMark); + milestoneMarkingOps.forEach(op => op.pipe(first()).subscribe({ // eslint-disable-next-line no-console - console.log("Marked activity as done", todo); - }); + next: (response) => console.log('Marked cascade milestone TodoItem as done:', response), + // eslint-disable-next-line no-console + error: (error) => console.error('Failed to mark cascade milestone TodoItem as done:', error) + })); + } }); + + // Fallback: if no duplicates found, try robust clearing for inaccurate data + if (result.duplicatesToMark.length === 0) { + const fallbackCleared = this.unlockIndicatorService.clearRelatedIndicators('activity', activity.id); + fallbackCleared?.forEach((todo) => { + this.notification + .markTodoItemAsDone(todo) + .pipe(first()) + .subscribe(() => { + // eslint-disable-next-line no-console + console.log("Marked activity as done (fallback)", todo); + }); + }); + } } if (this.unlockIndicatorService.isMilestoneClearable(milestone.id)) { @@ -318,18 +349,34 @@ export class HomePage implements OnInit, OnDestroy, AfterViewChecked { * @return {void} */ verifyUnlockedMilestoneValidity(milestoneId: number): void { - // check & update unlocked milestones - const unlockedMilestones = - this.unlockIndicatorService.clearActivity(milestoneId); - unlockedMilestones.forEach((unlockedMilestone) => { - this.notification - .markTodoItemAsDone(unlockedMilestone) - .pipe(first()) - .subscribe(() => { - // eslint-disable-next-line no-console - console.log("Marked milestone as done", unlockedMilestone); - }); - }); + // Use enhanced clearing that handles server-side duplicates + const currentTodoItems = this.notification.getCurrentTodoItems(); + const result = this.unlockIndicatorService.clearByMilestoneIdWithDuplicates(milestoneId, currentTodoItems); + + // Mark all duplicate TodoItems as done (bulk operation) + if (result.duplicatesToMark.length > 0) { + const markingOps = this.notification.markMultipleTodoItemsAsDone(result.duplicatesToMark); + markingOps.forEach(op => op.pipe(first()).subscribe({ + // eslint-disable-next-line no-console + next: (response) => console.log('Marked duplicate milestone TodoItem as done:', response), + // eslint-disable-next-line no-console + error: (error) => console.error('Failed to mark milestone TodoItem as done:', error) + })); + } + + // Fallback: if no duplicates found, try robust clearing for inaccurate data + if (result.duplicatesToMark.length === 0) { + const fallbackCleared = this.unlockIndicatorService.clearRelatedIndicators('milestone', milestoneId); + fallbackCleared.forEach((unlockedMilestone) => { + this.notification + .markTodoItemAsDone(unlockedMilestone) + .pipe(first()) + .subscribe(() => { + // eslint-disable-next-line no-console + console.log("Marked milestone as done (fallback)", unlockedMilestone); + }); + }); + } } async onTrackInfo() { diff --git a/projects/v3/src/app/services/notifications.service.ts b/projects/v3/src/app/services/notifications.service.ts index df0973043..0497ef2e9 100644 --- a/projects/v3/src/app/services/notifications.service.ts +++ b/projects/v3/src/app/services/notifications.service.ts @@ -471,6 +471,8 @@ export class NotificationsService { return this.modal(FastFeedbackComponent, props, modalConfig); } + private currentTodoItems: {id: number, identifier: string}[] = []; + getTodoItems(): Observable { return this.request .get(api.get.todoItem, { @@ -481,6 +483,15 @@ export class NotificationsService { .pipe( map((response) => { if (response.success && response.data) { + // Store current TodoItems for duplicate detection + this.currentTodoItems = response.data.map(item => ({ + id: item.id, + identifier: item.identifier + })); + + // Clean up orphaned unlock indicators before normalizing + this.unlockIndicatorService.cleanupOrphanedIndicators(response.data); + const normalised = this._normaliseTodoItems(response.data); this.notifications = normalised; this._notification$.next(this.notifications); @@ -490,6 +501,13 @@ export class NotificationsService { ); } + /** + * Get current TodoItems for duplicate detection + */ + getCurrentTodoItems(): {id: number, identifier: string}[] { + return this.currentTodoItems; + } + /** * group TodoItems into different types * - AssessmentReview @@ -1045,6 +1063,25 @@ export class NotificationsService { }); } + /** + * Mark multiple todo items as done (bulk operation) + * Handles server-side duplicates for same unlock indicator + */ + markMultipleTodoItemsAsDone(items: { identifier?: string; id?: number }[]) { + const markingOperations = items.map(item => + this.markTodoItemAsDone(item).pipe( + // Add error handling for individual items + map(response => ({ success: true, item, response })), + // Don't let individual failures stop the whole bulk operation + // catchError(error => of({ success: false, item, error })) + ) + ); + + // eslint-disable-next-line no-console + console.log(`Bulk marking ${items.length} TodoItems as done:`, items); + return markingOperations; + } + async trackInfo() { const modal = await this.modalController.create({ component: PopUpComponent, diff --git a/projects/v3/src/app/services/unlock-indicator.service.ts b/projects/v3/src/app/services/unlock-indicator.service.ts index 89b7bb496..5672152fe 100644 --- a/projects/v3/src/app/services/unlock-indicator.service.ts +++ b/projects/v3/src/app/services/unlock-indicator.service.ts @@ -1,5 +1,5 @@ import { Injectable } from '@angular/core'; -import { BehaviorSubject } from 'rxjs'; +import { BehaviorSubject, Observable } from 'rxjs'; import { BrowserStorageService } from './storage.service'; import { Activity } from './activity.service'; @@ -87,11 +87,235 @@ export class UnlockIndicatorService { } /** - * Clear all tasks related to a particular activity - * - * @param {number[]} id can either be activityId or milestoneId - * - * @return {UnlockedTask[]} unlocked tasks that were cleared + * Clear all tasks related to a particular activity (explicit) + * @param activityId + */ + clearByActivityId(activityId: number): UnlockedTask[] { + const current = this._unlockedTasksSubject.getValue(); + const cleared = current.filter(t => t.activityId === activityId); + const latest = current.filter(t => t.activityId !== activityId); + this.storageService.set('unlockedTasks', latest); + this._unlockedTasksSubject.next(latest); + return cleared; + } + + /** + * Enhanced clearing that handles duplicate TodoItems for same logical unlock + * Returns both cleared localStorage entries AND all duplicate TodoItems that need API marking + */ + clearByActivityIdWithDuplicates(activityId: number, currentTodoItems: {id: number, identifier: string}[]): { + clearedUnlocks: UnlockedTask[], + duplicatesToMark: {id: number, identifier: string}[], + cascadeMilestones: {milestoneId: number, duplicatesToMark: {id: number, identifier: string}[]}[] + } { + const current = this._unlockedTasksSubject.getValue(); + const activityUnlocks = current.filter(t => t.activityId === activityId); + + // Find all duplicate TodoItems for each unlocked task + let allDuplicatesToMark: {id: number, identifier: string}[] = []; + + activityUnlocks.forEach(unlockedTask => { + const duplicates = this.findDuplicateTodoItems(currentTodoItems, unlockedTask); + allDuplicatesToMark.push(...duplicates); + }); + + // Remove duplicates from the list + allDuplicatesToMark = allDuplicatesToMark.filter((item, index, self) => + index === self.findIndex(t => t.id === item.id) + ); + + // Clear from localStorage + const latest = current.filter(t => t.activityId !== activityId); + this.storageService.set('unlockedTasks', latest); + this._unlockedTasksSubject.next(latest); + + // Check for cascade milestone clearing + const cascadeMilestones: {milestoneId: number, duplicatesToMark: {id: number, identifier: string}[]}[] = []; + const affectedMilestones = new Set(activityUnlocks.map(t => t.milestoneId).filter(Boolean)); + + affectedMilestones.forEach(milestoneId => { + if (this.isMilestoneClearable(milestoneId)) { + const milestoneResult = this.clearByMilestoneIdWithDuplicates(milestoneId, currentTodoItems); + cascadeMilestones.push({ + milestoneId: milestoneId, + duplicatesToMark: milestoneResult.duplicatesToMark + }); + } + }); + + return { + clearedUnlocks: activityUnlocks, + duplicatesToMark: allDuplicatesToMark, + cascadeMilestones: cascadeMilestones + }; + } /** + * Clear all tasks related to a particular milestone (explicit) + * @param milestoneId + */ + clearByMilestoneId(milestoneId: number): UnlockedTask[] { + const current = this._unlockedTasksSubject.getValue(); + const cleared = current.filter(t => t.milestoneId === milestoneId); + const latest = current.filter(t => t.milestoneId !== milestoneId); + this.storageService.set('unlockedTasks', latest); + this._unlockedTasksSubject.next(latest); + return cleared; + } + + /** + * Enhanced milestone clearing that handles duplicate TodoItems + */ + clearByMilestoneIdWithDuplicates(milestoneId: number, currentTodoItems: {id: number, identifier: string}[]): { + clearedUnlocks: UnlockedTask[], + duplicatesToMark: {id: number, identifier: string}[] + } { + const current = this._unlockedTasksSubject.getValue(); + const milestoneUnlocks = current.filter(t => t.milestoneId === milestoneId); + + // Find all duplicate TodoItems for each unlocked task + let allDuplicatesToMark: {id: number, identifier: string}[] = []; + + milestoneUnlocks.forEach(unlockedTask => { + const duplicates = this.findDuplicateTodoItems(currentTodoItems, unlockedTask); + allDuplicatesToMark.push(...duplicates); + }); + + // Remove duplicates from the list + allDuplicatesToMark = allDuplicatesToMark.filter((item, index, self) => + index === self.findIndex(t => t.id === item.id) + ); + + // Clear from localStorage + const latest = current.filter(t => t.milestoneId !== milestoneId); + this.storageService.set('unlockedTasks', latest); + this._unlockedTasksSubject.next(latest); + + return { + clearedUnlocks: milestoneUnlocks, + duplicatesToMark: allDuplicatesToMark + }; + } + + /** + * Find related unlock indicators by entity type and id for robust cleanup + * This method handles inaccurate data by using fuzzy matching + */ + findRelatedIndicators(entityType: 'activity' | 'milestone' | 'task', entityId: number): UnlockedTask[] { + const current = this._unlockedTasksSubject.getValue(); + + switch (entityType) { + case 'activity': + // Find by activityId OR taskId that belongs to tasks in this activity + return current.filter(t => + t.activityId === entityId || + (t.taskId && this._isTaskInActivity(t.taskId, entityId)) + ); + + case 'milestone': + // Find by milestoneId OR activityId/taskId that belongs to this milestone + return current.filter(t => + t.milestoneId === entityId || + (t.activityId && this._isActivityInMilestone(t.activityId, entityId)) || + (t.taskId && this._isTaskInMilestone(t.taskId, entityId)) + ); + + case 'task': + // Find by taskId OR entries that should reference this task + return current.filter(t => + t.taskId === entityId || + (t.id && this._isRelatedToTask(t, entityId)) + ); + + default: + return []; + } + } + + /** + * Clear indicators with robust matching for inaccurate data + */ + clearRelatedIndicators(entityType: 'activity' | 'milestone' | 'task', entityId: number): UnlockedTask[] { + const current = this._unlockedTasksSubject.getValue(); + const toRemove = this.findRelatedIndicators(entityType, entityId); + const latest = current.filter(t => !toRemove.includes(t)); + + this.storageService.set('unlockedTasks', latest); + this._unlockedTasksSubject.next(latest); + + return toRemove; + } + + /** + * Clean up orphaned unlock indicators that no longer exist in current TodoItem API response + * This handles cases where localStorage has stale data that can't be marked as done via API + */ + cleanupOrphanedIndicators(currentTodoItems: {id: number, identifier: string}[]): UnlockedTask[] { + const current = this._unlockedTasksSubject.getValue(); + + const validIds = new Set(currentTodoItems.map(item => item.id)); + const validIdentifiers = new Set(currentTodoItems.map(item => item.identifier)); + + // Find orphaned entries that don't exist in current API response + const orphaned = current.filter(unlockedTask => { + // Check if this unlock indicator still exists in current TodoItem API response + const existsById = validIds.has(unlockedTask.id); + const existsByIdentifier = validIdentifiers.has(unlockedTask.identifier); + + // If neither ID nor identifier exists in current API, it's orphaned + return !existsById && !existsByIdentifier; + }); + + if (orphaned.length > 0) { + // Remove orphaned entries from localStorage + const cleaned = current.filter(t => !orphaned.includes(t)); + this.storageService.set('unlockedTasks', cleaned); + this._unlockedTasksSubject.next(cleaned); + + // eslint-disable-next-line no-console + console.log(`Cleaned up ${orphaned.length} orphaned unlock indicators:`, orphaned); + } + + return orphaned; + } + + /** + * Find and return all duplicate TodoItems for the same logical unlock + * This handles cases where server creates multiple TodoItems for same unlocked item + */ + findDuplicateTodoItems(currentTodoItems: {id: number, identifier: string}[], unlockedTask: UnlockedTask): {id: number, identifier: string}[] { + // Group TodoItems by base identifier (without unique suffixes) + const baseIdentifier = unlockedTask.identifier.replace(/-\d+$/, ''); // Remove trailing numbers if any + + // Find all TodoItems with similar identifiers or same logical unlock + return currentTodoItems.filter(item => { + // Match by exact identifier + if (item.identifier === unlockedTask.identifier) return true; + + // Match by base identifier pattern (e.g., "NewItem-17432" matches "NewItem-17432-1", "NewItem-17432-2") + const itemBaseIdentifier = item.identifier.replace(/-\d+$/, ''); + if (itemBaseIdentifier === baseIdentifier) return true; + + // Match by identifier prefix for same unlock event + if (item.identifier.startsWith(baseIdentifier)) return true; + + return false; + }); + } + + /** + * Bulk clear all duplicate TodoItems for a given unlock indicator + * Returns array of TodoItems that need to be marked as done externally + */ + bulkClearDuplicates(unlockedTask: UnlockedTask, allDuplicates: {id: number, identifier: string}[]): {id: number, identifier: string}[] { + if (allDuplicates.length > 0) { + // eslint-disable-next-line no-console + console.log(`Found ${allDuplicates.length} duplicate TodoItems for unlock:`, unlockedTask, allDuplicates); + } + + return allDuplicates; + } + + /** + * Deprecated: use clearByActivityId or clearByMilestoneId */ clearActivity(id: number): UnlockedTask[] { const currentTasks = this._unlockedTasksSubject.getValue(); @@ -105,6 +329,30 @@ export class UnlockIndicatorService { return clearedActivities; } + // Helper methods for fuzzy matching (these would need actual implementation based on your data relationships) + private _isTaskInActivity(taskId: number, activityId: number): boolean { + // This would need to check if taskId belongs to activityId + // Could be implemented by checking against current activity data or making a lookup + return false; // Placeholder - implement based on your data structure + } + + private _isActivityInMilestone(activityId: number, milestoneId: number): boolean { + // This would check if activityId belongs to milestoneId + return false; // Placeholder - implement based on your data structure + } + + private _isTaskInMilestone(taskId: number, milestoneId: number): boolean { + // This would check if taskId belongs to milestoneId through its activity + return false; // Placeholder - implement based on your data structure + } + + private _isRelatedToTask(unlockedTask: UnlockedTask, taskId: number): boolean { + // Check if the unlocked task is somehow related to the given taskId + // This could check identifier patterns, meta data, etc. + return unlockedTask.identifier?.includes(`Task-${taskId}`) || + unlockedTask.meta?.task_id === taskId; + } + getTasksByMilestoneId(milestoneId: number): UnlockedTask[] { return this._unlockedTasksSubject.getValue().filter(unlocked => unlocked.milestoneId === milestoneId); } From 1851eb534ee34ce6bc4bbca31e89d1b4293d489f Mon Sep 17 00:00:00 2001 From: trtshen Date: Thu, 28 Aug 2025 16:34:55 +0800 Subject: [PATCH 2/3] [CORE-6673] code touch up --- .../activity-desktop/activity-desktop.page.ts | 31 +--- projects/v3/src/app/pages/home/home.page.ts | 43 +----- .../src/app/services/notifications.service.ts | 77 ++++++---- .../app/services/unlock-indicator.service.ts | 140 +++++++++++------- 4 files changed, 147 insertions(+), 144 deletions(-) diff --git a/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts b/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts index 35ff0bd91..47fe98c34 100644 --- a/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts +++ b/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts @@ -327,36 +327,7 @@ export class ActivityDesktopPage { // handles server-side duplicates and hierarchy const result = this.unlockIndicatorService.clearByActivityIdWithDuplicates(activityId, currentTodoItems); - // Mark all duplicate TodoItems as done (bulk operation) - if (result.duplicatesToMark.length > 0) { - const markingOps = this.notificationsService.markMultipleTodoItemsAsDone(result.duplicatesToMark); - markingOps.forEach(op => op.subscribe({ - // eslint-disable-next-line no-console - next: (response) => console.log('Marked duplicate activity TodoItem as done:', response), - // eslint-disable-next-line no-console - error: (error) => console.error('Failed to mark activity TodoItem as done:', error) - })); - } - - // handles cascade milestone clearing - result.cascadeMilestones.forEach(milestoneData => { - if (milestoneData.duplicatesToMark.length > 0) { - // eslint-disable-next-line no-console - console.log(`Cascade clearing milestone ${milestoneData.milestoneId} with ${milestoneData.duplicatesToMark.length} duplicates`); - const milestoneMarkingOps = this.notificationsService.markMultipleTodoItemsAsDone(milestoneData.duplicatesToMark); - milestoneMarkingOps.forEach(op => op.subscribe({ - // eslint-disable-next-line no-console - next: (response) => console.log('Marked cascade milestone TodoItem as done:', response), - // eslint-disable-next-line no-console - error: (error) => console.error('Failed to mark cascade milestone TodoItem as done:', error) - })); - } - }); - - // Fallback: mark cleared localStorage items as done (for backward compatibility) - result.clearedUnlocks?.forEach(todo => { - this.notificationsService.markTodoItemAsDone(todo).subscribe(); - }); + this.unlockIndicatorService.markDuplicatesAsDone(result, this.notificationsService, 'activity'); return; } diff --git a/projects/v3/src/app/pages/home/home.page.ts b/projects/v3/src/app/pages/home/home.page.ts index 829d33b2a..5e7906930 100644 --- a/projects/v3/src/app/pages/home/home.page.ts +++ b/projects/v3/src/app/pages/home/home.page.ts @@ -322,31 +322,8 @@ export class HomePage implements OnInit, OnDestroy, AfterViewChecked { const currentTodoItems = this.notification.getCurrentTodoItems(); const result = this.unlockIndicatorService.clearByActivityIdWithDuplicates(activity.id, currentTodoItems); - // Mark all duplicate TodoItems as done (bulk operation) - if (result.duplicatesToMark.length > 0) { - const markingOps = this.notification.markMultipleTodoItemsAsDone(result.duplicatesToMark); - markingOps.forEach(op => op.pipe(first()).subscribe({ - // eslint-disable-next-line no-console - next: (response) => console.log('Marked duplicate activity TodoItem as done:', response), - // eslint-disable-next-line no-console - error: (error) => console.error('Failed to mark activity TodoItem as done:', error) - })); - } - - // Handle cascade milestone clearing - result.cascadeMilestones.forEach(milestoneData => { - if (milestoneData.duplicatesToMark.length > 0) { - // eslint-disable-next-line no-console - console.log(`Cascade clearing milestone ${milestoneData.milestoneId} with ${milestoneData.duplicatesToMark.length} duplicates`); - const milestoneMarkingOps = this.notification.markMultipleTodoItemsAsDone(milestoneData.duplicatesToMark); - milestoneMarkingOps.forEach(op => op.pipe(first()).subscribe({ - // eslint-disable-next-line no-console - next: (response) => console.log('Marked cascade milestone TodoItem as done:', response), - // eslint-disable-next-line no-console - error: (error) => console.error('Failed to mark cascade milestone TodoItem as done:', error) - })); - } - }); + // Handle marking duplicate TodoItems as done using centralized method + this.unlockIndicatorService.markDuplicatesAsDone(result, this.notification, 'activity'); // Fallback: if no duplicates found, try robust clearing for inaccurate data if (result.duplicatesToMark.length === 0) { @@ -380,22 +357,14 @@ export class HomePage implements OnInit, OnDestroy, AfterViewChecked { * @return {void} */ verifyUnlockedMilestoneValidity(milestoneId: number): void { - // Use enhanced clearing that handles server-side duplicates + // handles server-side duplicates clearing const currentTodoItems = this.notification.getCurrentTodoItems(); const result = this.unlockIndicatorService.clearByMilestoneIdWithDuplicates(milestoneId, currentTodoItems); - // Mark all duplicate TodoItems as done (bulk operation) - if (result.duplicatesToMark.length > 0) { - const markingOps = this.notification.markMultipleTodoItemsAsDone(result.duplicatesToMark); - markingOps.forEach(op => op.pipe(first()).subscribe({ - // eslint-disable-next-line no-console - next: (response) => console.log('Marked duplicate milestone TodoItem as done:', response), - // eslint-disable-next-line no-console - error: (error) => console.error('Failed to mark milestone TodoItem as done:', error) - })); - } + // mark all duplicated TodoItems as done + this.unlockIndicatorService.markDuplicatesAsDone(result, this.notification, 'milestone'); - // Fallback: if no duplicates found, try robust clearing for inaccurate data + // Fallback: if no duplicates found, try clearing for inaccurate unlock indicator todoItems if (result.duplicatesToMark.length === 0) { const fallbackCleared = this.unlockIndicatorService.clearRelatedIndicators('milestone', milestoneId); fallbackCleared.forEach((unlockedMilestone) => { diff --git a/projects/v3/src/app/services/notifications.service.ts b/projects/v3/src/app/services/notifications.service.ts index 4acd5c5dd..b2d282944 100644 --- a/projects/v3/src/app/services/notifications.service.ts +++ b/projects/v3/src/app/services/notifications.service.ts @@ -49,6 +49,49 @@ export interface Meta { assessment_name: string; } +export interface TodoItemMeta { + // feedback/assessment related properties + timeline_id?: number; + assessment_id?: number | string; // can be number or string in some cases + submission_id?: number; + context_id?: number; + activity_id?: number; + submitter_name?: string; + assessment_name?: string; + published_date?: string; // iso date string + reviewer_name?: string; + + // achievement related properties + id?: number; + name?: string; + description?: string | null; + badge?: string; // url to badge image + points?: number; + program_id?: number; + experience_id?: number; + new_items?: any[]; // array of new items unlocked + + // chat/fast feedback related properties + team_id?: number | null; + team_name?: string; + target_user_id?: number; + + // reminder related properties + due_date?: string | null; // iso date string or null + + // unlock/hierarchy related properties + parent_milestone?: number; + parent_activity?: number; + task_type?: string; // "Story.Topic", "Assess.Assessment", etc. + task_id?: number | null; + + // legacy/unknown properties + participants_only?: boolean; + team_member_id?: number; + Unlock?: any; // legacy property, type unclear + assessment_submission_id?: number; +} + /** * TodoItem interface * @description: this object can be very dynamic. It acts as a notification object for the user. @@ -64,27 +107,7 @@ export interface TodoItem { is_done?: boolean; foreign_key?: number; // milestoneId/activitySequenceId/activityId model?: string; - meta?: { - id?: number; - name?: string; - description?: string; - points?: number; - badge?: string; - activity_id?: number; - context_id?: number; - assessment_id?: number; - assessment_submission_id?: number; - assessment_name?: string; - reviewer_name?: string; - team_id?: number; - team_member_id?: number; - participants_only?: boolean; - due_date?: string; - task_id?: number; - task_type?: string; - parent_activity?: number; // a referrence to the parent activity id for task - parent_milestone?: number; // a referrence to the parent activity id for task - }; + meta?: TodoItemMeta; project_id?: number; timeline_id?: number; } @@ -482,10 +505,15 @@ export class NotificationsService { .pipe( map((response) => { if (response.success && response.data) { + const todoItems: TodoItem[] = response.data; + // Store current TodoItems for duplicate detection - this.currentTodoItems = response.data.map(item => ({ + this.currentTodoItems = todoItems + .filter(item => item.is_done === false) + .map(item => ({ id: item.id, - identifier: item.identifier + identifier: item.identifier, + is_done: item.is_done })); // Clean up orphaned unlock indicators before normalizing @@ -1069,10 +1097,7 @@ export class NotificationsService { markMultipleTodoItemsAsDone(items: { identifier?: string; id?: number }[]) { const markingOperations = items.map(item => this.markTodoItemAsDone(item).pipe( - // Add error handling for individual items map(response => ({ success: true, item, response })), - // Don't let individual failures stop the whole bulk operation - // catchError(error => of({ success: false, item, error })) ) ); diff --git a/projects/v3/src/app/services/unlock-indicator.service.ts b/projects/v3/src/app/services/unlock-indicator.service.ts index 5672152fe..596a9cf81 100644 --- a/projects/v3/src/app/services/unlock-indicator.service.ts +++ b/projects/v3/src/app/services/unlock-indicator.service.ts @@ -1,7 +1,9 @@ import { Injectable } from '@angular/core'; import { BehaviorSubject, Observable } from 'rxjs'; +import { first } from 'rxjs/operators'; import { BrowserStorageService } from './storage.service'; -import { Activity } from './activity.service'; +import { Activity, ActivityService } from './activity.service'; +import { NotificationsService } from './notifications.service'; export interface UnlockedTask { id?: number; @@ -28,9 +30,7 @@ export enum UnlockIndicatorModel { providedIn: 'root' }) export class UnlockIndicatorService { - // Initialize with an empty array private _unlockedTasksSubject = new BehaviorSubject([]); - // Expose as an observable for components to subscribe public unlockedTasks$ = this._unlockedTasksSubject.asObservable(); constructor( @@ -148,7 +148,9 @@ export class UnlockIndicatorService { duplicatesToMark: allDuplicatesToMark, cascadeMilestones: cascadeMilestones }; - } /** + } + + /** * Clear all tasks related to a particular milestone (explicit) * @param milestoneId */ @@ -301,49 +303,45 @@ export class UnlockIndicatorService { }); } - /** - * Bulk clear all duplicate TodoItems for a given unlock indicator - * Returns array of TodoItems that need to be marked as done externally - */ - bulkClearDuplicates(unlockedTask: UnlockedTask, allDuplicates: {id: number, identifier: string}[]): {id: number, identifier: string}[] { - if (allDuplicates.length > 0) { - // eslint-disable-next-line no-console - console.log(`Found ${allDuplicates.length} duplicate TodoItems for unlock:`, unlockedTask, allDuplicates); + // fuzzy matching of unlock indicator todoItems + private _isTaskInActivity(taskId: number, activityId: number): boolean { + // Since we can't directly access the current activity synchronously, + // we'll rely on the relationships stored in unlocked tasks (localstorage) + const tasks = this._unlockedTasksSubject.getValue(); + + // 1st: Check if direct relationship exists + const hasDirectRelationship = tasks.some(t => t.taskId === taskId && t.activityId === activityId); + if (hasDirectRelationship) { + return true; } - return allDuplicates; - } - - /** - * Deprecated: use clearByActivityId or clearByMilestoneId - */ - clearActivity(id: number): UnlockedTask[] { - const currentTasks = this._unlockedTasksSubject.getValue(); - - const clearedActivities = currentTasks.filter(task => task.activityId === id || task.milestoneId === id); - const latestTasks = currentTasks.filter(task => task.activityId !== id && task.milestoneId !== id); - - this.storageService.set('unlockedTasks', latestTasks); - this._unlockedTasksSubject.next(latestTasks); - - return clearedActivities; - } - - // Helper methods for fuzzy matching (these would need actual implementation based on your data relationships) - private _isTaskInActivity(taskId: number, activityId: number): boolean { - // This would need to check if taskId belongs to activityId - // Could be implemented by checking against current activity data or making a lookup - return false; // Placeholder - implement based on your data structure + // 2nd approach: check if there are any tasks from this activity + // and if this taskId appears in the same activity context + const tasksInActivity = tasks.filter(t => t.activityId === activityId); + return tasksInActivity.some(t => t.taskId === taskId); } private _isActivityInMilestone(activityId: number, milestoneId: number): boolean { - // This would check if activityId belongs to milestoneId - return false; // Placeholder - implement based on your data structure + const existingTasks = this._unlockedTasksSubject.getValue(); + return existingTasks.some(t => t.activityId === activityId && t.milestoneId === milestoneId); } private _isTaskInMilestone(taskId: number, milestoneId: number): boolean { - // This would check if taskId belongs to milestoneId through its activity - return false; // Placeholder - implement based on your data structure + const existingTasks = this._unlockedTasksSubject.getValue(); + + // Method 1: Direct task-milestone relationship (if it exists) + const directRelationship = existingTasks.some(t => t.taskId === taskId && t.milestoneId === milestoneId); + if (directRelationship) { + return true; + } + + // Method 2: Task belongs to an activity that belongs to this milestone + // Find tasks that have all three: taskId, activityId, and milestoneId + const taskWithFullHierarchy = existingTasks.find(t => + t.taskId === taskId && t.activityId !== undefined && t.milestoneId === milestoneId + ); + + return !!taskWithFullHierarchy; } private _isRelatedToTask(unlockedTask: UnlockedTask, taskId: number): boolean { @@ -353,6 +351,55 @@ export class UnlockIndicatorService { unlockedTask.meta?.task_id === taskId; } + /** + * Mark multiple duplicated TodoItems as done for clearing results + */ + markDuplicatesAsDone( + result: { + duplicatesToMark: {id: number, identifier: string}[], + cascadeMilestones?: {milestoneId: number, duplicatesToMark: {id: number, identifier: string}[]}[], + clearedUnlocks?: UnlockedTask[] + }, + notificationsService: NotificationsService, // pass in service to avoid circular dependency + context: string = 'activity' + ): void { + // mark duplicated TodoItems as done (bulk operation) + if (result.duplicatesToMark.length > 0) { + const markingOps = notificationsService.markMultipleTodoItemsAsDone(result.duplicatesToMark); + markingOps.forEach(op => op.pipe(first()).subscribe({ + // eslint-disable-next-line no-console + next: (response) => console.log(`Marked duplicate ${context} TodoItem as done:`, response), + // eslint-disable-next-line no-console + error: (error) => console.error(`Failed to mark ${context} TodoItem as done:`, error) + })); + } + + // cascade to milestone clearing + result.cascadeMilestones?.forEach(milestoneData => { + if (milestoneData.duplicatesToMark.length > 0) { + // eslint-disable-next-line no-console + console.log(`Cascade clearing milestone ${milestoneData.milestoneId} with ${milestoneData.duplicatesToMark.length} duplicates`); + const milestoneMarkingOps = notificationsService.markMultipleTodoItemsAsDone(milestoneData.duplicatesToMark); + milestoneMarkingOps.forEach(op => op.pipe(first()).subscribe({ + // eslint-disable-next-line no-console + next: (response) => console.log('Marked cascade milestone TodoItem as done:', response), + // eslint-disable-next-line no-console + error: (error) => console.error('Failed to mark cascade milestone TodoItem as done:', error) + })); + } + }); + + // Fallback: mark cleared localStorage items as done (for backward compatibility) + result.clearedUnlocks?.forEach(todo => { + notificationsService.markTodoItemAsDone(todo).pipe(first()).subscribe({ + // eslint-disable-next-line no-console + next: (response) => console.log('Marked fallback TodoItem as done:', response), + // eslint-disable-next-line no-console + error: (error) => console.error('Failed to mark fallback TodoItem as done:', error) + }); + }); + } + getTasksByMilestoneId(milestoneId: number): UnlockedTask[] { return this._unlockedTasksSubject.getValue().filter(unlocked => unlocked.milestoneId === milestoneId); } @@ -383,16 +430,7 @@ export class UnlockIndicatorService { this._unlockedTasksSubject.next(uniquelatestTasks); } - // Method to remove an accessed tasks - // (some tasks are repeatable due to unlock from different level of trigger eg. by milestone, activity, task) - // removeTasks(taskId?: number): UnlockedTask[] { - // const currentTasks = this._unlockedTasksSubject.getValue(); - // const removedTask = currentTasks.filter(task => task.taskId === taskId); - // const latestTasks = currentTasks.filter(task => task.taskId !== taskId); - // this.storageService.set('unlockedTasks', latestTasks); - // this._unlockedTasksSubject.next(latestTasks); - // return removedTask; - // } + removeTasks(taskId?: number): UnlockedTask[] { const currentTasks = this._unlockedTasksSubject.getValue(); @@ -440,7 +478,7 @@ export class UnlockIndicatorService { return removedTasks; } - // Method to transform and deduplicate the data + // transform and deduplicate the data transformAndDeduplicate(data) { const uniqueEntries = new Map(); @@ -456,7 +494,7 @@ export class UnlockIndicatorService { } }); - // Convert the map values to an array + // Convert to array return Array.from(uniqueEntries.values()); } } From a1cefb13e296375f2b3c7c7585acafed41a5accb Mon Sep 17 00:00:00 2001 From: trtshen Date: Fri, 22 Aug 2025 16:39:39 +0800 Subject: [PATCH 3/3] [CORE-6673] always pull latest todo-item for more accurate marking --- docs/unlock-indicator.md | 165 ++++++++++++++++-- .../components/activity/activity.component.ts | 29 +-- .../activity-desktop/activity-desktop.page.ts | 114 +++++++++--- projects/v3/src/app/pages/home/home.page.ts | 8 +- .../app/services/navigation-state.service.ts | 25 +++ 5 files changed, 293 insertions(+), 48 deletions(-) create mode 100644 projects/v3/src/app/services/navigation-state.service.ts diff --git a/docs/unlock-indicator.md b/docs/unlock-indicator.md index 8a391435a..ece475eab 100644 --- a/docs/unlock-indicator.md +++ b/docs/unlock-indicator.md @@ -2,10 +2,32 @@ This document explains how the “unlock indicator” (red dot) is implemented, how it’s stored and updated, and how it appears in the UI across the Home page and list items. +## Files involved +- Service: `projects/v- `activity.component.ts` + - Subscribes to `unlockIndicatorService.unlockedTasks$` and builds a `newTasks` map keyed by `taskId` to flag per-task "new/unlocked" state inside the activity view. + - Uses `distinctUntilChanged` to prevent unnecessary updates when unlock data hasn't actually changed. + - Only updates visual indicators without triggering any clearing logic when new unlocks arrive. + - Preserves newly unlocked task indicators until user explicitly clicks on them. + +- `activity-desktop.page.ts` & `activity-mobile.page.ts` + - Include page-enter cleanup logic (`_clearActivityLevelIndicators`) that respects hierarchical clearing rules. + - Only clear activity-level indicators when no task-level children remain (`isActivityClearable()`). + - Use enhanced duplicate detection and bulk TodoItem marking for reliable clearing. + - Handle both navigation from Home and direct activity entry scenarios. + +- `navigation-state.service.ts` + - **NEW**: Provides persistent navigation source tracking across routing boundaries. + - Solves the routing hierarchy issue where `router.getCurrentNavigation()` returns `null` after navigation completes. + - Used by Home page to set navigation source before navigation, and Activity pages to check source after navigation. + - Simple set/check/clear pattern: `setNavigationSource('home')` → `isFromSource('home')` → `clearNavigationSource()`. + ## Files involved - Service: `projects/v3/src/app/services/unlock-indicator.service.ts` +- Service: `projects/v3/src/app/services/navigation-state.service.ts` *(NEW - navigation state tracking)* - Home page (TS): `projects/v3/src/app/pages/home/home.page.ts` - Home page (HTML): `projects/v3/src/app/pages/home/home.page.html` +- Activity component (TS): `projects/v3/src/app/components/activity/activity.component.ts` +- Activity pages: `projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts` - List item component (HTML): `projects/v3/src/app/components/list-item/list-item.component.html` ## Concept overview @@ -93,15 +115,15 @@ findDuplicateTodoItems(currentTodoItems, unlockedTask) { return currentTodoItems.filter(item => { // Exact identifier match if (item.identifier === unlockedTask.identifier) return true; - + // Base identifier pattern matching (handles variations) const baseIdentifier = unlockedTask.identifier.replace(/-\d+$/, ''); const itemBaseIdentifier = item.identifier.replace(/-\d+$/, ''); if (itemBaseIdentifier === baseIdentifier) return true; - + // Prefix matching for same unlock event if (item.identifier.startsWith(baseIdentifier)) return true; - + return false; }); } @@ -115,11 +137,11 @@ clearByActivityIdWithDuplicates(activityId, currentTodoItems) { // 1. Clear activity and find all duplicates const activityResult = this.clearActivity(activityId); const duplicates = this.findAllDuplicates(activityResult); - + // 2. Check affected parent milestones const affectedMilestones = new Set(activityResult.map(t => t.milestoneId)); const cascadeMilestones = []; - + affectedMilestones.forEach(milestoneId => { if (this.isMilestoneClearable(milestoneId)) { // 3. Auto-clear parent milestone if it becomes clearable @@ -127,7 +149,7 @@ clearByActivityIdWithDuplicates(activityId, currentTodoItems) { cascadeMilestones.push(milestoneResult); } }); - + return { duplicates, cascadeMilestones }; } ``` @@ -145,7 +167,7 @@ localStorage: [ ``` **When user visits activity 26686**: -1. **Activity Clearing**: +1. **Activity Clearing**: - Finds duplicates: `[25480, 25479]` for "NewItem-17434" - Marks both as done via bulk API calls - Removes activity entry from localStorage @@ -155,7 +177,7 @@ localStorage: [ - Auto-triggers milestone clearing 3. **Milestone Clearing**: - - Finds duplicates: `[25473, 25475, 25474]` for "NewItem-17432" + - Finds duplicates: `[25473, 25475, 25474]` for "NewItem-17432" - Marks all as done via bulk API calls - Removes milestone entry from localStorage @@ -178,7 +200,7 @@ getTodoItems() { map(response => { // Clean up stale localStorage entries before processing this.unlockIndicatorService.cleanupOrphanedIndicators(response.data); - + const normalised = this._normaliseTodoItems(response.data); return normalised; }) @@ -302,7 +324,7 @@ No additional logic is required in the list item; it purely reflects the `redDot private _clearPureActivityIndicator() { const activityLevelEntries = this.unlockIndicatorService.getTasksByActivityId(this.activity.id) .filter(task => task.taskId === undefined); // Only pure activity entries - + if (activityLevelEntries.length > 0 && this.unlockIndicatorService.isActivityClearable(this.activity.id)) { const result = this.unlockIndicatorService.clearByActivityIdWithDuplicates(this.activity.id, this.currentTodoItems); // Mark duplicates as done via bulk API calls @@ -336,15 +358,75 @@ if (entry.taskId && !entry.activityId) { - `markMultipleTodoItemsAsDone()` handles bulk API marking - `cleanupOrphanedIndicators()` removes stale localStorage entries +#### 5. Navigation State Loss in Activity Pages +**Problem**: `router.getCurrentNavigation()` returns `null` after navigation completes, preventing proper clearing decision. + +**Solution**: NavigationStateService for persistent navigation tracking: +```typescript +// Home page - before navigation +this.navigationStateService.setNavigationSource('home'); +this.router.navigate(['v3', 'activity-desktop', activityId]); + +// Activity page - after navigation +const fromHome = this.navigationStateService.isFromSource('home'); +this.navigationStateService.clearNavigationSource(); +``` + +#### 6. Task-Level Indicators Not Showing When User Already in Activity +**Problem**: When user is viewing an activity and new tasks get unlocked, the red dots don't appear. + +**Solution**: Activity component reactive updates: +- Subscribe to `unlockedTasks$` with `distinctUntilChanged()` +- Update visual indicators without triggering clearing logic +- Preserve new task indicators until user clicks on them +```typescript +// activity.component.ts +ngOnInit() { + this.unlockIndicatorService.unlockedTasks$ + .pipe(distinctUntilChanged(), takeUntil(this.unsubscribe$)) + .subscribe(res => { + // Only update visual indicators, don't clear anything + if (this.activity?.id) { + const activityUnlocks = this.unlockIndicatorService.getTasksByActivity(this.activity); + this.resetTaskIndicator(activityUnlocks); + } + }); +} +``` + +#### 7. Activity-Level Indicators Not Clearing Due to Strict Hierarchy +**Problem**: Activity-level indicators persist because the condition `entries.every(e => e.taskId === undefined)` is too restrictive. + +**Solution**: Separate handling of activity-level vs task-level entries: +```typescript +const activityLevelEntries = entries.filter(e => e.taskId === undefined); +const taskLevelEntries = entries.filter(e => e.taskId !== undefined); + +// Only clear activity-level when no task-level children exist +if (activityLevelEntries.length > 0 && taskLevelEntries.length === 0) { + // Safe to clear activity-level indicators +} +``` + ### Implementation Checklist for Robustness -- [ ] **Activity Pages**: Add page-enter cleanup for activity-level-only entries +- [x] **NavigationStateService**: Persistent navigation source tracking across routing boundaries + - File: `projects/v3/src/app/services/navigation-state.service.ts` + - Resolves navigation state loss issues with `router.getCurrentNavigation()` +- [x] **Activity Component Reactive Updates**: Preserve newly unlocked task indicators + - File: `projects/v3/src/app/components/activity/activity.component.ts` + - Uses `distinctUntilChanged()` and only updates visual indicators +- [x] **Activity Pages**: Add page-enter cleanup for activity-level-only entries - Desktop: `projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts` - Mobile: Equivalent activity page files + - Implements hierarchical clearing with `_clearActivityLevelIndicators()` +- [x] **Enhanced Hierarchy Logic**: Separate activity-level and task-level entry handling + - Prevents overly restrictive clearing conditions + - Only clears activity-level when no task-level children remain - [ ] **Service Methods**: Replace ambiguous `clearActivity` with explicit methods - `clearByActivityId(activityId: number)` - `clearByMilestoneId(milestoneId: number)` -- [ ] **Data Validation**: Enforce `activityId` presence for task entries +- [x] **Data Validation**: Enforce `activityId` presence for task entries - File: `projects/v3/src/app/services/notifications.service.ts` - [ ] **Route Guards**: Optional resolver-based cleanup on activity routes - [ ] **Testing**: Unit tests for new methods and e2e tests for deep links @@ -371,6 +453,11 @@ Enhanced methods provide detailed console output: 3. **Deep Link to Task**: Ensure task and parent clearing works 4. **Milestone Clearing**: Verify cascade clearing when all children visited 5. **Experience Switch**: Confirm `clearAllTasks()` resets all state +6. **User Already in Activity**: Test that newly unlocked tasks show red dots immediately +7. **Navigation State Persistence**: Verify NavigationStateService works across routing boundaries +8. **Hierarchical Clearing**: Test that activity-level indicators only clear when no task children remain +9. **Server Duplicate Handling**: Verify bulk TodoItem marking clears all duplicates +10. **Activity Updates While Viewing**: Ensure new unlocks appear without false clearing ### Performance Considerations - **Bulk Operations**: Parallel TodoItem marking reduces API overhead @@ -378,6 +465,60 @@ Enhanced methods provide detailed console output: - **Cascade Logic**: Smart parent clearing reduces manual intervention - **Pattern Matching**: Efficient duplicate detection with regex patterns +## Routing Hierarchy and Navigation State Issues + +### Problem: Navigation State Loss +Angular's `router.getCurrentNavigation()` only returns navigation data **during** the navigation process. Once navigation completes and components load, it returns `null`. This creates issues when Activity pages need to determine their navigation source for clearing decisions. + +**Symptom**: Activity-level unlock indicators don't clear when navigating from Home because the navigation state is lost by the time the Activity page's `ionViewDidEnter()` executes. + +### Routing Structure Complexity +``` +/v3/tabs +├── /home (Home page) +└── /activity-desktop/:id (Activity page) +``` + +The Home and Activity pages are siblings under the tabs router, not parent-child. This means navigation state passed via `router.navigate(['path'], { state: {...} })` gets lost during the tab routing process. + +### Solution: NavigationStateService +A persistent service that tracks navigation source across routing boundaries: + +```typescript +@Injectable({ providedIn: 'root' }) +export class NavigationStateService { + private navigationSource$ = new BehaviorSubject(null); + + setNavigationSource(source: string) { /* ... */ } + isFromSource(source: string): boolean { /* ... */ } + clearNavigationSource() { /* ... */ } +} +``` + +**Implementation Pattern**: +1. **Home page** (before navigation): `navigationStateService.setNavigationSource('home')` +2. **Activity page** (after navigation): `isFromHome = navigationStateService.isFromSource('home')` +3. **Activity page** (after reading): `navigationStateService.clearNavigationSource()` + +**Benefits**: +- Works across any routing configuration (tabs, lazy-loaded modules, etc.) +- Independent of Angular's navigation lifecycle timing +- Simple and predictable behavior +- Reliable alternative to transient navigation objects + +### Activity Page Entry Points +Activity pages can be entered via multiple paths: +- **Home → Activity**: Should clear activity-level indicators (if clearable) +- **Direct URL/Deep Link**: Should clear activity-level indicators (if clearable) +- **Task → Back → Activity**: Should not re-clear already cleared indicators +- **Notification → Activity**: Should clear activity-level indicators (if clearable) + +The `_clearActivityLevelIndicators()` method handles all entry points by: +1. Checking if activity has activity-level entries to clear +2. Verifying no task-level children remain (`isActivityClearable()`) +3. Using enhanced duplicate detection for reliable clearing +4. Auto-cascading to parent milestones when they become clearable + ## Edge cases and notes - **Hierarchy enforcement**: Activity-level clearing is intentionally conservative - it only happens when there are no task-level unlocks (`isActivityClearable` returns `true`). If any task under the activity remains unlocked, the red dot persists. - **Milestone clearability**: Milestone indicators are NOT manually clearable - they only clear when all their children (activities and tasks) have been cleared. diff --git a/projects/v3/src/app/components/activity/activity.component.ts b/projects/v3/src/app/components/activity/activity.component.ts index cb20ebe1a..8e5fa9942 100644 --- a/projects/v3/src/app/components/activity/activity.component.ts +++ b/projects/v3/src/app/components/activity/activity.component.ts @@ -8,7 +8,7 @@ import { Submission } from '@v3/services/assessment.service'; import { NotificationsService } from '@v3/services/notifications.service'; import { BrowserStorageService } from '@v3/services/storage.service'; import { UtilsService } from '@v3/services/utils.service'; -import { takeUntil } from 'rxjs/operators'; +import { takeUntil, distinctUntilChanged } from 'rxjs/operators'; @Component({ selector: 'app-activity', @@ -58,9 +58,20 @@ export class ActivityComponent implements OnInit, OnChanges, OnDestroy { ngOnInit() { this.leadImage = this.storageService.getUser().programImage; this.unlockIndicatorService.unlockedTasks$ - .pipe(takeUntil(this.unsubscribe$)) + .pipe( + takeUntil(this.unsubscribe$), + distinctUntilChanged((prev, curr) => JSON.stringify(prev) === JSON.stringify(curr)) + ) .subscribe({ - next: res => this.resetTaskIndicator(res) + next: res => { + // only update the visual indicators, don't clear anything + if (this.activity?.id) { + const activityUnlocks = this.unlockIndicatorService.getTasksByActivity(this.activity); + this.resetTaskIndicator(activityUnlocks); + } else { + this.resetTaskIndicator(res); + } + } }); } @@ -112,19 +123,9 @@ export class ActivityComponent implements OnInit, OnChanges, OnDestroy { this.cannotAccessTeamActivity.emit(this.isForTeamOnly); }); - // clear viewed unlocked indicator + // update unlock indicators when activity changes, but don't clear const unlockedTasks = this.unlockIndicatorService.getTasksByActivity(this.activity); this.resetTaskIndicator(unlockedTasks); - if (unlockedTasks.length === 0) { - // handle inaccurate unlock indicators - const clearedActivities = this.unlockIndicatorService.clearRelatedIndicators('activity', this.activity.id); - clearedActivities.forEach((activity) => { - this.notificationsService - .markTodoItemAsDone(activity) - .pipe(takeUntil(this.unsubscribe$)) - .subscribe(); - }); - } } } } diff --git a/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts b/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts index 47fe98c34..fef6a1a96 100644 --- a/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts +++ b/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts @@ -42,6 +42,9 @@ export class ActivityDesktopPage { }; scrolSubject = new BehaviorSubject(null); + // track navigation state for unlock indicator clearing + private fromHome: boolean = false; + @ViewChild(AssessmentComponent) assessmentComponent!: AssessmentComponent; @ViewChild('scrollableTaskContent', { static: false }) scrollableTaskContent: {el: HTMLIonColElement}; @ViewChild(TopicComponent) topicComponent: TopicComponent; @@ -108,6 +111,10 @@ export class ActivityDesktopPage { // cleanup previous session this.componentCleanupService.triggerCleanup(); + // capture navigation state early before it's lost + const navigation = this.router.getCurrentNavigation(); + this.fromHome = navigation?.extras?.state?.fromHome || false; + this.activityService.activity$ .pipe( filter((res) => res?.id === +this.route.snapshot.paramMap.get('id')), @@ -302,41 +309,109 @@ export class ActivityDesktopPage { } }); } - // Clear pure activity-level unlock indicators on page enter/update - this._clearPureActivityIndicator(res.id); return; } this.activity = res; - // Clear pure activity-level unlock indicators on initial set - this._clearPureActivityIndicator(res.id); + // only clear pure activity-level unlock indicators onLoad of activity when navigating from Home + this._clearPureActivityIndicatorIfFromHome(res.id); + } + + /** + * clears activity-level unlock indicators only when navigating from Home page + */ + private _clearPureActivityIndicatorIfFromHome(activityId: number): void { + if (!activityId) { return; } + + // check if user is navigating from Home page using stored state + if (!this.fromHome) { + return; + } + + this._clearActivityLevelIndicators(activityId); } /** - * clears activity-level unlock indicators on page enter + * checks if activity-level indicators should be cleared after task completion + * called when user completes tasks within the activity */ - private _clearPureActivityIndicator(activityId: number): void { + private _checkActivityLevelClearingAfterTaskCompletion(): void { + if (!this.activity?.id) { + return; + } + + // use timeout to allow unlock indicator service to update after task completion + setTimeout(() => { + this._clearActivityLevelIndicators(this.activity.id); + }, 500); + } + + private async _clearActivityLevelIndicators(activityId: number): Promise { if (!activityId) { return; } try { - // First try the enhanced approach that handles duplicates const currentTodoItems = this.notificationsService.getCurrentTodoItems(); - const entries = this.unlockIndicatorService.getTasksByActivityId(activityId); + let entries = this.unlockIndicatorService.getTasksByActivityId(activityId); - if (entries?.length > 0 && entries.every(e => e.taskId === undefined)) { - // handles server-side duplicates and hierarchy + // retry fetching todo items if no entries found + if (entries?.length === 0) { + await firstValueFrom(this.notificationsService.getTodoItems()); + entries = this.unlockIndicatorService.getTasksByActivityId(activityId); + } + + // Double confirmed, no indicators for this activity + // if (entries?.length > 0 && entries.every(e => e.taskId === undefined)) { + // // handles server-side duplicates and hierarchy + // const result = this.unlockIndicatorService.clearByActivityIdWithDuplicates(activityId, currentTodoItems); + + // this.unlockIndicatorService.markDuplicatesAsDone(result, this.notificationsService, 'activity'); + if (!entries || entries.length === 0) { + return; + } + + // Separate activity-level and task-level indicators + const activityLevelEntries = entries.filter(e => e.taskId === undefined); + const taskLevelEntries = entries.filter(e => e.taskId !== undefined); + + // Only clear activity-level indicators if: + // 1. There are activity-level entries to clear + // 2. The activity is clearable (no task-level children) + if (activityLevelEntries.length > 0 && taskLevelEntries.length === 0) { + // Activity is clearable - no task children remain const result = this.unlockIndicatorService.clearByActivityIdWithDuplicates(activityId, currentTodoItems); - this.unlockIndicatorService.markDuplicatesAsDone(result, this.notificationsService, 'activity'); + // Mark the original cleared activity-level indicators as done + result.clearedUnlocks?.forEach(todo => { + this.notificationsService.markTodoItemAsDone(todo).subscribe(() => { + // eslint-disable-next-line no-console + console.info("Marked activity indicator as done (activity page)", todo); + }); + }); + + // Mark all duplicate TodoItems as done (bulk operation) + if (result.duplicatesToMark.length > 0) { + this.notificationsService.markMultipleTodoItemsAsDone(result.duplicatesToMark); + } + + // Handle cascade milestone clearing + result.cascadeMilestones.forEach(milestoneData => { + if (milestoneData.duplicatesToMark.length > 0) { + const milestoneMarkingOps = this.notificationsService.markMultipleTodoItemsAsDone(milestoneData.duplicatesToMark); + } + }); + + // Note: The fallback at line 364-367 was already handling this, but only as a fallback return; } - // If standard approach didn't find anything, try robust clearing for inaccurate data - const relatedIndicators = this.unlockIndicatorService.findRelatedIndicators('activity', activityId); - if (relatedIndicators?.length > 0) { - // Only clear if they are pure activity-level (no task-specific entries) + // If we couldn't clear via standard approach, try robust clearing + // This handles inaccurate data where relationships might be broken + if (activityLevelEntries.length > 0) { + const relatedIndicators = this.unlockIndicatorService.findRelatedIndicators('activity', activityId); const pureActivityIndicators = relatedIndicators.filter(r => r.taskId === undefined); - if (pureActivityIndicators.length > 0) { + + // Only clear if activity is truly clearable (no tasks) + if (pureActivityIndicators.length > 0 && taskLevelEntries.length === 0) { const cleared = this.unlockIndicatorService.clearRelatedIndicators('activity', activityId); cleared?.forEach(todo => { this.notificationsService.markTodoItemAsDone(todo).subscribe(); @@ -344,9 +419,7 @@ export class ActivityDesktopPage { } } } catch (e) { - // swallow to avoid breaking page enter; optional logging can be added under dev flag - // eslint-disable-next-line no-console - console.debug('[unlock-indicator] cleanup skipped for activity', activityId, e); + console.error('[unlock-indicator] cleanup failed for activity', activityId, e); } } @@ -382,6 +455,7 @@ export class ActivityDesktopPage { } await this.activityService.goToTask(task); + this._checkActivityLevelClearingAfterTaskCompletion(); this.isLoadingAssessment = false; } catch (error) { this.isLoadingAssessment = false; @@ -532,7 +606,7 @@ export class ActivityDesktopPage { delay(400) )); await this.reviewRatingPopUp(); - await this.notificationsService.getTodoItems().toPromise(); // update notifications list + await firstValueFrom(this.notificationsService.getTodoItems()); // update notifications list this.loading = false; this.btnDisabled$.next(false); diff --git a/projects/v3/src/app/pages/home/home.page.ts b/projects/v3/src/app/pages/home/home.page.ts index 5e7906930..0edc071ef 100644 --- a/projects/v3/src/app/pages/home/home.page.ts +++ b/projects/v3/src/app/pages/home/home.page.ts @@ -5,6 +5,7 @@ import { Achievement, AchievementService, } from '@v3/app/services/achievement.service'; +import { NavigationStateService } from '@v3/app/services/navigation-state.service'; import { NotificationsService } from '@v3/app/services/notifications.service'; import { SharedService } from '@v3/app/services/shared.service'; import { BrowserStorageService } from '@v3/app/services/storage.service'; @@ -69,6 +70,7 @@ export class HomePage implements OnInit, OnDestroy, AfterViewChecked { private sharedService: SharedService, private storageService: BrowserStorageService, private unlockIndicatorService: UnlockIndicatorService, + private navigationStateService: NavigationStateService, private cdr: ChangeDetectorRef, private fastFeedbackService: FastFeedbackService, private alertController: AlertController, @@ -325,8 +327,8 @@ export class HomePage implements OnInit, OnDestroy, AfterViewChecked { // Handle marking duplicate TodoItems as done using centralized method this.unlockIndicatorService.markDuplicatesAsDone(result, this.notification, 'activity'); - // Fallback: if no duplicates found, try robust clearing for inaccurate data - if (result.duplicatesToMark.length === 0) { + // Fallback: if no duplicates found, try to clear inaccurate data + if (result.duplicatesToMark.length === 0 && result.clearedUnlocks.length === 0) { const fallbackCleared = this.unlockIndicatorService.clearRelatedIndicators('activity', activity.id); fallbackCleared?.forEach((todo) => { this.notification @@ -345,6 +347,8 @@ export class HomePage implements OnInit, OnDestroy, AfterViewChecked { } if (!this.isMobile) { + // manually set navigation source + this.navigationStateService.setNavigationSource('home'); return this.router.navigate(["v3", "activity-desktop", activity.id]); } diff --git a/projects/v3/src/app/services/navigation-state.service.ts b/projects/v3/src/app/services/navigation-state.service.ts new file mode 100644 index 000000000..7124b7914 --- /dev/null +++ b/projects/v3/src/app/services/navigation-state.service.ts @@ -0,0 +1,25 @@ +import { Injectable } from '@angular/core'; +import { BehaviorSubject } from 'rxjs'; + +@Injectable({ + providedIn: 'root' +}) +export class NavigationStateService { + private navigationSource$ = new BehaviorSubject(null); + + setNavigationSource(source: string) { + this.navigationSource$.next(source); + } + + getNavigationSource(): string | null { + return this.navigationSource$.value; + } + + clearNavigationSource() { + this.navigationSource$.next(null); + } + + isFromSource(source: string): boolean { + return this.getNavigationSource() === source; + } +}