diff --git a/docs/fixes/dead-code-audit-notificationsservice.md b/docs/fixes/dead-code-audit-notificationsservice.md new file mode 100644 index 000000000..c4d9cdd65 --- /dev/null +++ b/docs/fixes/dead-code-audit-notificationsservice.md @@ -0,0 +1,145 @@ +# Dead Code Audit: NotificationsService and PopUpComponent + +## Date: February 4, 2026 + +## Scope +Audit of unused modal methods and modal types in: +- `projects/v3/src/app/services/notifications.service.ts` +- `projects/v3/src/app/components/pop-up/pop-up.component.html` + +## Findings + +### Removed in This Session + +#### 1. `trackInfo()` Method ✅ REMOVED +- **Location**: `notifications.service.ts` (lines 1055-1064) +- **Purpose**: Display pulse check status information modal +- **Status**: **UNUSED** - superseded by home page local implementation +- **Evidence**: + - Home page uses local `onTrackInfo()` method instead + - No references found in codebase (except deleted method) + - Not in documentation or API contracts +- **Action**: Removed + +#### 2. `pulseCheckStatus` Modal Type ✅ REMOVED +- **Location**: `pop-up.component.html` (lines 48-59) +- **Purpose**: Template case for pulse check status info +- **Status**: **UNUSED** - only referenced by deleted `trackInfo()` method +- **Evidence**: + - Only caller was `trackInfo()` method + - Home page traffic light display uses different binding (`pulseCheckStatus` data property, not modal type) + - No other references in codebase +- **Action**: Removed + +### Active Modal Methods (All Used) + +#### Public Modal Creation Methods +1. **`popUp(type, data, redirect)`** ✅ USED + - Used by: auth-forgot-password, auth-registration, contact-number-form, home page + - Creates PopUpComponent modals with types: `forgotPasswordConfirmation`, `shortMessage`, `guidelines` + +2. **`achievementPopUp(type, achievement, options)`** ✅ USED + - Used by: home page, shared service, notifications service (for achievement notifications) + - Creates AchievementPopUpComponent modals + +3. **`lockTeamAssessmentPopUp(data, event)`** ✅ USED + - Used by: assessment components when user clicks locked team assessment + - Creates LockTeamAssessmentPopUpComponent modals + +4. **`activityCompletePopUp(activityId, activityCompleted)`** ✅ USED + - Used by: activity service when activity is completed + - Creates ActivityCompletePopUpComponent modals + +5. **`popUpReviewRating(reviewId, redirect, hasReviewRating)`** ✅ USED + - Used by: activity-desktop, assessment-mobile, devtool pages + - Creates ReviewRatingComponent modals + +6. **`fastFeedbackModal(props, options)`** ✅ USED + - Used by: fast-feedback service for pulse check + - Creates FastFeedbackComponent modals + +#### Supporting Methods +7. **`modal(component, componentProps, options, event)`** ✅ USED + - Internal wrapper around modalOnly + - Used by all modal creation methods + +8. **`modalOnly(component, componentProps, options, event)`** ✅ USED + - Core modal creation with queue management + - Uses ModalService to handle modal queue + +#### Utility Methods +9. **`alert(config)`** ✅ USED + - Used throughout app for alert dialogs + - AlertController wrapper + +10. **`presentToast(message, options)`** ✅ USED + - Used throughout app for toast notifications + - ToastController wrapper + +11. **`loading(opts)`** ✅ USED + - Used for loading spinners + - LoadingController wrapper + +12. **`dismiss()`** ✅ USED + - Used to dismiss modals + - ModalController wrapper + +### Active PopUpComponent Modal Types (All Used) + +1. **`forgotPasswordConfirmation`** ✅ USED + - Used by: auth-forgot-password component + - Shows email confirmation message + +2. **`shortMessage`** ✅ USED + - Used by: contact-number-form, auth-registration, home page + - Generic short message display + +3. **`guidelines`** ✅ USED + - Used by: home page + - Shows message with clickable route links + +## Audit Summary + +- **Total methods audited**: 12 public methods + 2 internal helpers +- **Unused methods found**: 1 (`trackInfo()`) +- **Unused modal types found**: 1 (`pulseCheckStatus`) +- **All other methods**: Actively used and verified + +## Verification Method + +1. Grep search for method names across entire codebase +2. Check documentation files for references +3. Verify template usage in PopUpComponent +4. Cross-reference with home page implementation + +## Recommendations + +### Immediate Actions ✅ COMPLETED +- Remove `trackInfo()` method from notifications.service.ts +- Remove `pulseCheckStatus` case from pop-up.component.html +- Update notifications.service.spec.ts to remove any tests for `trackInfo()` + +### Future Considerations + +1. **No additional dead code found**: All other modal methods are actively used +2. **Modal queue pattern is healthy**: ModalService properly manages modal display queue +3. **PopUpComponent types are lean**: Only 3 modal types remain, all in active use + +## Testing Impact + +### Test Files Affected +- `projects/v3/src/app/services/notifications.service.spec.ts` + - Remove any tests for `trackInfo()` method if present + +### No Tests Expected For +- `pulseCheckStatus` modal type (template-only change) + +## Related Documentation + +See [review-rating-duplicate-api-fix.md](./review-rating-duplicate-api-fix.md) for context on why this audit was performed. + +## Notes + +- Home page's local `onTrackInfo()` implementation uses AlertController directly instead of NotificationsService +- This is a better pattern as it keeps the UI logic local to the page component +- No breaking changes expected from removals as code was genuinely unused diff --git a/docs/fixes/review-rating-duplicate-api-fix.md b/docs/fixes/review-rating-duplicate-api-fix.md new file mode 100644 index 000000000..c3706e09d --- /dev/null +++ b/docs/fixes/review-rating-duplicate-api-fix.md @@ -0,0 +1,167 @@ +# Review Rating Duplicate API Call Fix + +## Issue Summary + +When the review rating modal is displayed after feedback reading, the pulse check API (`pullFastFeedback()`) was being called twice: +1. Once by the ReviewRatingComponent when the modal is dismissed +2. Once by the page component (activity-desktop/assessment-mobile) after `reviewRatingPopUp()` returns + +This resulted in unnecessary duplicate GraphQL API calls, impacting performance and potentially causing race conditions. + +## Root Cause + +The original implementation added an explicit `pullFastFeedback()` call in the page components to fix the case where the review rating modal was skipped (when `assessment.hasReviewRating === false`). However, when the modal IS shown, the ReviewRatingComponent's `dismissModal()` method already calls `pullFastFeedback()` through `fastFeedbackOrRedirect()`, resulting in the duplicate call. + +## Solution + +### Approach + +**Conditional pulse check execution**: Only call `pullFastFeedback()` from page components when the review rating modal was actually skipped. + +- **When modal is skipped** (user/assessment has review rating disabled): Page component calls `pullFastFeedback()` +- **When modal is shown**: ReviewRatingComponent handles `pullFastFeedback()` on dismiss (no longer - see architectural change below) + +### Architectural Change + +To follow separation of concerns and match the pattern used in assessment submissions: +- **Removed** pulse check responsibility from ReviewRatingComponent +- **Centralized** pulse check calls in page components (activity-desktop, assessment-mobile) +- ReviewRatingComponent now only handles redirect navigation + +## Implementation Flow + +### Before Fix + +```mermaid +sequenceDiagram + participant User + participant Page as Activity/Assessment Page + participant Modal as ReviewRatingComponent + participant API as FastFeedbackService + + alt Review Rating Enabled + User->>Page: Read feedback + Page->>Modal: reviewRatingPopUp() + Modal->>User: Show modal + User->>Modal: Dismiss + Modal->>API: pullFastFeedback() ❌ Call #1 + Modal->>Page: Return + Page->>API: pullFastFeedback() ❌ Call #2 (DUPLICATE) + else Review Rating Disabled + User->>Page: Read feedback + Page->>Page: reviewRatingPopUp() returns early + Page->>API: pullFastFeedback() ✅ Call #1 + end +``` + +### After Fix + +```mermaid +sequenceDiagram + participant User + participant Page as Activity/Assessment Page + participant Modal as ReviewRatingComponent + participant API as FastFeedbackService + + alt Review Rating Enabled + User->>Page: Read feedback + Page->>Modal: reviewRatingPopUp() + Modal->>User: Show modal + User->>Modal: Dismiss (rate or skip) + Modal->>Page: Return (redirect only if needed) + Note over Page: Check: was modal skipped? + Page->>Page: hasReviewRating is TRUE + Page->>API: pullFastFeedback() ✅ Call #1 (no duplicate) + else Review Rating Disabled + User->>Page: Read feedback + Page->>Page: reviewRatingPopUp() returns early + Note over Page: Check: was modal skipped? + Page->>Page: hasReviewRating is FALSE + Page->>API: pullFastFeedback() ✅ Call #1 + end +``` + +## Files Modified + +### Code Changes + +1. **projects/v3/src/app/pages/activity-desktop/activity-desktop.page.ts** + - Added conditional check in `readFeedback()` method + - Only calls `pullFastFeedback()` when `hasReviewRating === false` + +2. **projects/v3/src/app/pages/assessment-mobile/assessment-mobile.page.ts** + - Applied identical conditional logic in `readFeedback()` method + - Ensures mobile and desktop behavior is consistent + +3. **projects/v3/src/app/components/review-rating/review-rating.component.ts** + - Refactored `fastFeedbackOrRedirect()` method + - Removed pulse check call when `!redirect` + - Now only handles redirect navigation + +### Dead Code Removal + +4. **projects/v3/src/app/services/notifications.service.ts** + - Removed unused `trackInfo()` method (lines 1055-1064) + - Method was superseded by home page's local `onTrackInfo()` implementation + +5. **projects/v3/src/app/components/pop-up/pop-up.component.html** + - Removed unused `pulseCheckStatus` modal case (lines 48-59) + - Only referenced by deleted `trackInfo()` method + +## Testing Approach + +### Manual Testing Scenarios + +1. **Review rating enabled (user level + assessment level)** + - Read feedback → Review rating modal shows → Dismiss + - Verify pulse check modal appears once + - Check network tab for single `pullFastFeedback` GraphQL call + +2. **Review rating disabled (user level)** + - Read feedback → No review rating modal + - Verify pulse check modal appears immediately + - Check network tab for single `pullFastFeedback` GraphQL call + +3. **Review rating disabled (assessment level)** + - Read feedback → No review rating modal + - Verify pulse check modal appears immediately + - Check network tab for single `pullFastFeedback` GraphQL call + +4. **Review rating with redirect** + - Complete review rating → Submit with redirect + - Verify navigation to next task works correctly + - Verify pulse check appears before redirect + +### Test Files to Update + +- `projects/v3/src/app/pages/activity-desktop/activity-desktop.page.spec.ts` +- `projects/v3/src/app/pages/assessment-mobile/assessment-mobile.page.spec.ts` +- `projects/v3/src/app/components/review-rating/review-rating.component.spec.ts` +- `projects/v3/src/app/services/notifications.service.spec.ts` + +## Performance Impact + +**Before**: 2 GraphQL API calls per review rating flow (when modal shown) +**After**: 1 GraphQL API call per review rating flow + +**Estimated savings**: 50% reduction in pulse check API calls for review rating flows where modal is displayed + +## Edge Cases Handled + +1. **User-level review rating disabled**: `storageService.getUser().hasReviewRating === false` +2. **Assessment-level review rating disabled**: `assessmentService.assessment?.hasReviewRating === false` +3. **Redirect scenarios**: Review rating component still handles redirect navigation correctly +4. **No redirect scenarios**: Pulse check now handled by page components + +## Related Changes + +- Initial implementation: Added `hasReviewRating` parameter to `popUpReviewRating()` method +- Bug fix: Added explicit `pullFastFeedback()` calls when modal skipped +- Optimization: This fix to prevent duplicate calls when modal shown +- Cleanup: Removed unused `trackInfo()` method and `pulseCheckStatus` modal type + +## Future Considerations + +- Consider returning modal reference from `popUpReviewRating()` if more modal state detection is needed +- Monitor for similar duplicate API call patterns in other modal workflows +- Consider extracting modal lifecycle detection into a reusable pattern/service diff --git a/projects/v3/src/app/components/pop-up/pop-up.component.html b/projects/v3/src/app/components/pop-up/pop-up.component.html index c4b0bf598..7580bb5d5 100644 --- a/projects/v3/src/app/components/pop-up/pop-up.component.html +++ b/projects/v3/src/app/components/pop-up/pop-up.component.html @@ -45,19 +45,6 @@ - - - - -