Merge pull request #2627 from intersective/prerelease

shawnm0705 · web-flow · commit d7c4f15ccebc · 2026-03-03T14:41:19.000+08:00
Release 2.4.7.3
diff --git a/.gitignore b/.gitignore
@@ -27,3 +27,4 @@ www/*
 .env
 projects/v3/src/environments/environment.ts
 projects/v3/src/environments/environment.prod.ts
+output/
diff --git a/docs/fixes/CORE-8002-pulsecheck-workflow.md b/docs/fixes/CORE-8002-pulsecheck-workflow.md
@@ -0,0 +1,83 @@
+# CORE-8002: Pulse check popup displayed twice
+
+## issue summary
+In a moderated assessment flow (`feedback available` -> mark review as read -> continue), learners could receive the same pulse check popup twice:
+1. first popup after feedback/read + review rating flow
+2. second popup after navigating to Home
+
+## root cause
+`fastFeedbackOpening` was cleared too early in `FastFeedbackService.pullFastFeedback()`.
+
+- the lock was set before opening the modal
+- it was reset in `finalize()` right after modal creation/enqueue (not after modal dismissal)
+- during this gap, another pulse check check (for example Home `updateDashboard()`) could run and queue/open the same pulse check again
+
+## fix applied
+Updated `projects/v3/src/app/services/fast-feedback.service.ts`:
+- moved `retry` before `switchMap` so retries only re-run the API fetch, never the modal open
+- replaced `catchError` with `finalize` on the modal observable so the lock is released on success, error, and unsubscribe
+
+### operator ordering change
+
+```
+before (retry after switchMap — retried modal opens):
+  fetch() → switchMap(modal()) → retry(3)
+
+after (retry before switchMap — only retries fetch):
+  fetch() → retry(3) → switchMap(modal())
+```
+
+`retry` re-subscribes to everything **above** it in the pipe. placing it before `switchMap` means a fetch error retries the fetch, but a modal error passes through without re-triggering the entire pipeline.
+
+## lock state diagram
+
+the `fastFeedbackOpening` storage flag acts as a mutex to prevent concurrent modal opens:
+
+```
+         pullFastFeedback() called
+                │
+                ▼
+        ┌───────────────┐
+        │ lock = true?  │──── YES ──→ skip modal, return of(res)
+        └───────┬───────┘
+                │ NO
+                ▼
+        set lock = TRUE           ← acquire
+                │
+                ▼
+        open modal popup
+                │
+        ┌───────┴────────┐
+        │                │
+    success           error/dismiss
+        │                │
+        ▼                ▼
+    finalize()       finalize()    ← both paths release
+        │                │
+        ▼                ▼
+    set lock = FALSE              ← release
+```
+
+`finalize` runs when the observable completes, errors, or is unsubscribed — covering all exit paths:
+- user submits pulse check → observable completes → lock released
+- modal throws error → observable errors → lock released
+- user navigates away (component destroyed) → subscription unsubscribed → lock released
+
+## why this is safe
+- preserves existing behavior for successful pulse check display
+- prevents duplicate modal opens from concurrent trigger paths
+- `finalize` guarantees lock release on every exit path — no deadlock possible
+
+## reference trigger paths observed
+- review rating close path may request pulse check (`ReviewRatingComponent.fastFeedbackOrRedirect`)
+- Home dashboard load also requests pulse check (`HomePage.updateDashboard`)
+- assessment submission path (`AssessmentService._afterSubmit`)
+- review submission path (`ReviewDesktopPage`)
+- manual user click on traffic light (`TrafficLightGroupComponent.navigateToPulseCheck`)
+
+the first two can fire back-to-back in the reported reproduce steps.
+
+## verification checklist
+- reproduce original scenario with moderated assessment feedback-read flow
+- confirm only one pulse check popup is shown
+- confirm pulse check can still be shown later when a genuinely new pulse check is available
diff --git a/projects/v3/src/app/components/fast-feedback/fast-feedback.component.spec.ts b/projects/v3/src/app/components/fast-feedback/fast-feedback.component.spec.ts
@@ -84,9 +84,17 @@ describe('FastFeedbackComponent', () => {
     expect(Object.keys(component.fastFeedbackForm.controls).length).toBe(5);
   });
 
-  it('when testing dismiss(), it should dismiss', () => {
+  it('when testing dismiss(), it should dismiss and release lock', () => {
+    const storageSpy = TestBed.inject(BrowserStorageService) as jasmine.SpyObj<BrowserStorageService>;
     component.dismiss({});
     expect(modalSpy.dismiss.calls.count()).toBe(1);
+    expect(storageSpy.set).toHaveBeenCalledWith('fastFeedbackOpening', false);
+  });
+
+  it('ngOnDestroy() should release fastFeedbackOpening lock', () => {
+    const storageSpy = TestBed.inject(BrowserStorageService) as jasmine.SpyObj<BrowserStorageService>;
+    component.ngOnDestroy();
+    expect(storageSpy.set).toHaveBeenCalledWith('fastFeedbackOpening', false);
   });
 
   describe('when testing submit()', () => {
diff --git a/projects/v3/src/app/components/fast-feedback/fast-feedback.component.ts b/projects/v3/src/app/components/fast-feedback/fast-feedback.component.ts
@@ -279,6 +279,10 @@ export class FastFeedbackComponent implements OnInit, OnDestroy {
   ngOnDestroy(): void {
     // Clean up ESC key listener
     document.removeEventListener('keydown', this.handleKeyDown);
+
+    // safety: release the lock if the component is destroyed without dismiss
+    // (e.g. user navigates away while modal is still open)
+    this.storage.set('fastFeedbackOpening', false);
   }
 
   get isRedColor(): boolean {
diff --git a/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.html b/projects/v3/src/app/pages/activity-desktop/activity-desktop.page.html
@@ -42,7 +42,8 @@
     </ion-row>
   </ion-grid>
 
-  <ng-container *ngIf="currentTask?.type === 'Assessment'">
+  <ng-container *ngIf="currentTask?.type === 'Assessment' && false">
+    <!-- temporary hide fab button - assessment question navigator -->
     <span class="tooltip-container"
       [ngStyle]="tooltipStyle"
       *ngIf="tooltipVisible">
diff --git a/projects/v3/src/app/services/fast-feedback.service.spec.ts b/projects/v3/src/app/services/fast-feedback.service.spec.ts
@@ -1,18 +1,37 @@
-import { TestBed } from '@angular/core/testing';
+import { TestBed, fakeAsync, tick } from '@angular/core/testing';
 import { FastFeedbackService } from './fast-feedback.service';
-import { of, throwError } from 'rxjs';
-import { RequestService } from 'request';
+import { of } from 'rxjs';
 import { TestUtils } from '@testingv3/utils';
 import { NotificationsService } from '@v3/services/notifications.service';
 import { BrowserStorageService } from '@v3/services/storage.service';
 import { UtilsService } from '@v3/services/utils.service';
+import { DemoService } from './demo.service';
+import { ApolloService } from './apollo.service';
+
+// helper to build a valid pulse check API response
+function makePulseCheckResponse(questions: any[] = [], meta: any = null) {
+  return {
+    data: {
+      pulseCheck: {
+        questions,
+        meta,
+      }
+    }
+  };
+}
+
+const VALID_QUESTIONS = [
+  { id: 7, name: 'Q1', choices: [{ id: 1, name: 'Yes' }, { id: 2, name: 'No' }] },
+  { id: 8, name: 'Q2', choices: [{ id: 3, name: 'Yes' }, { id: 4, name: 'No' }] },
+];
+
+const VALID_META = { teamId: 100, teamName: 'Team A', contextId: 200 };
 
 describe('FastFeedbackService', () => {
   let service: FastFeedbackService;
-  let requestSpy: jasmine.SpyObj<RequestService>;
+  let apolloSpy: jasmine.SpyObj<ApolloService>;
   let notificationSpy: jasmine.SpyObj<NotificationsService>;
   let storageSpy: jasmine.SpyObj<BrowserStorageService>;
-  const testUtils = new TestUtils();
 
   beforeEach(() => {
     TestBed.configureTestingModule({
@@ -23,21 +42,30 @@ describe('FastFeedbackService', () => {
           useClass: TestUtils,
         },
         {
-          provide: RequestService,
-          useValue: jasmine.createSpyObj('RequestService', ['get', 'post'])
+          provide: ApolloService,
+          useValue: jasmine.createSpyObj('ApolloService', {
+            graphQLFetch: of({}),
+            graphQLMutate: of({}),
+          }),
         },
         {
           provide: NotificationsService,
-          useValue: jasmine.createSpyObj('NotificationsService', ['modal'])
+          useValue: jasmine.createSpyObj('NotificationsService', {
+            fastFeedbackModal: Promise.resolve(),
+          }),
         },
         {
           provide: BrowserStorageService,
-          useValue: jasmine.createSpyObj('BrowserStorageService', ['set', 'get'])
-        }
+          useValue: jasmine.createSpyObj('BrowserStorageService', ['set', 'get']),
+        },
+        {
+          provide: DemoService,
+          useValue: jasmine.createSpyObj('DemoService', ['fastFeedback', 'normalResponse']),
+        },
       ]
     });
     service = TestBed.inject(FastFeedbackService);
-    requestSpy = TestBed.inject(RequestService) as jasmine.SpyObj<RequestService>;
+    apolloSpy = TestBed.inject(ApolloService) as jasmine.SpyObj<ApolloService>;
     notificationSpy = TestBed.inject(NotificationsService) as jasmine.SpyObj<NotificationsService>;
     storageSpy = TestBed.inject(BrowserStorageService) as jasmine.SpyObj<BrowserStorageService>;
   });
@@ -46,99 +74,102 @@ describe('FastFeedbackService', () => {
     expect(service).toBeTruthy();
   });
 
-  it('should get fastfeedback from API', () => {
-    requestSpy.get.and.returnValue(of({}));
-    service["_getFastFeedback"]().subscribe();
-    expect(requestSpy.get.calls.count()).toBe(1);
+  it('should fetch pulse check data from API', () => {
+    apolloSpy.graphQLFetch.and.returnValue(of({}));
+    service['_getFastFeedback']().subscribe();
+    expect(apolloSpy.graphQLFetch).toHaveBeenCalledTimes(1);
   });
 
-  /*it('should open fastfeedback modal', () => {
-    service.fastFeedbackModal();
-    expect(notificationSpy.modal.calls.count()).toBe(1);
-  });*/
-
   describe('when testing pullFastFeedback()', () => {
-    it('should pop up modal', () => {
-      requestSpy.get.and.returnValue(of({
-        data: {
-          slider: {
-            length: 1
-          },
-          meta: {
-            any: 'data'
-          }
-        }
-      }));
-      storageSpy.get.and.returnValue(false);
-      service.pullFastFeedback().subscribe(res => {
-        expect(storageSpy.set.calls.count()).toBe(1);
-        expect(notificationSpy.modal.calls.count()).toBe(1);
+    it('should open modal and set lock when pulse check data is valid', () => {
+      apolloSpy.graphQLFetch.and.returnValue(of(makePulseCheckResponse(VALID_QUESTIONS, VALID_META)));
+      storageSpy.get.and.returnValue(false); // fastFeedbackOpening = false
+
+      service.pullFastFeedback().subscribe(() => {
+        // should set fastFeedbackOpening = true
+        expect(storageSpy.set).toHaveBeenCalledWith('fastFeedbackOpening', true);
+        // should call fastFeedbackModal
+        expect(notificationSpy.fastFeedbackModal).toHaveBeenCalledTimes(1);
       });
     });
 
-    it('should not pop up modal when slider object length is 0', () => {
-      requestSpy.get.and.returnValue(of({
-        data: {
-          slider: {
-            length: 0
-          }
-        }
-      }));
+    it('should NOT release the lock after modal is opened (fire-and-forget)', fakeAsync(() => {
+      apolloSpy.graphQLFetch.and.returnValue(of(makePulseCheckResponse(VALID_QUESTIONS, VALID_META)));
       storageSpy.get.and.returnValue(false);
-      service.pullFastFeedback().subscribe(res => {
-        expect(storageSpy.set.calls.count()).toBe(0);
-        expect(notificationSpy.modal.calls.count()).toBe(0);
+
+      service.pullFastFeedback().subscribe();
+      tick();
+
+      // lock is set to true and never released by the service
+      const setCalls = storageSpy.set.calls.allArgs();
+      const lockCalls = setCalls.filter(args => args[0] === 'fastFeedbackOpening');
+      expect(lockCalls.length).toBe(1);
+      expect(lockCalls[0]).toEqual(['fastFeedbackOpening', true]);
+    }));
+
+    it('should not open modal when fastFeedbackOpening is already true', () => {
+      apolloSpy.graphQLFetch.and.returnValue(of(makePulseCheckResponse(VALID_QUESTIONS, VALID_META)));
+      storageSpy.get.and.returnValue(true); // lock already held
+
+      service.pullFastFeedback().subscribe(() => {
+        expect(notificationSpy.fastFeedbackModal).not.toHaveBeenCalled();
       });
     });
 
-    it('should not pop up modal when get storage returns false', () => {
-      requestSpy.get.and.returnValue(throwError(''));
+    it('should not open modal when pulseCheck data is empty', () => {
+      apolloSpy.graphQLFetch.and.returnValue(of({ data: { pulseCheck: null } }));
       storageSpy.get.and.returnValue(false);
-      service.pullFastFeedback().subscribe(res => {
-        expect(storageSpy.set.calls.count()).toBe(0);
-        expect(notificationSpy.modal.calls.count()).toBe(0);
+
+      service.pullFastFeedback().subscribe(() => {
+        expect(notificationSpy.fastFeedbackModal).not.toHaveBeenCalled();
       });
     });
 
-    it('should not popup modal when slider & meta are not available', () => {
-      requestSpy.get.and.returnValue(of({
-        data: {
-          slider: undefined,
-          meta: undefined,
-        }
-      }));
+    it('should not open modal when questions are empty', () => {
+      apolloSpy.graphQLFetch.and.returnValue(of(makePulseCheckResponse([], VALID_META)));
+      storageSpy.get.and.returnValue(false);
 
-      service.pullFastFeedback().subscribe(res => {
-        expect(notificationSpy.modal).not.toHaveBeenCalled();
+      service.pullFastFeedback().subscribe(() => {
+        expect(notificationSpy.fastFeedbackModal).not.toHaveBeenCalled();
       });
     });
 
-    it('should not popup modal when slider is not available', () => {
-      requestSpy.get.and.returnValue(of({
-        data: {
-          slider: [],
-          meta: { hasValue: true },
-        }
-      }));
+    it('should not open modal when meta is empty', () => {
+      apolloSpy.graphQLFetch.and.returnValue(of(makePulseCheckResponse(VALID_QUESTIONS, null)));
+      storageSpy.get.and.returnValue(false);
 
-      service.pullFastFeedback().subscribe(res => {
-        expect(notificationSpy.modal).not.toHaveBeenCalled();
+      service.pullFastFeedback().subscribe(() => {
+        expect(notificationSpy.fastFeedbackModal).not.toHaveBeenCalled();
       });
     });
 
-    it('should not popup modal when meta is not available', () => {
-      requestSpy.get.and.returnValue(of({
-        data: {
-          slider: [1, 2],
-          meta: undefined,
-        }
-      }));
+    it('should release lock on modal open error', fakeAsync(() => {
+      apolloSpy.graphQLFetch.and.returnValue(of(makePulseCheckResponse(VALID_QUESTIONS, VALID_META)));
+      storageSpy.get.and.returnValue(false);
+      notificationSpy.fastFeedbackModal.and.returnValue(Promise.reject('modal error'));
+
+      service.pullFastFeedback().subscribe();
+      tick(); // resolve rejected promise
+
+      const setCalls = storageSpy.set.calls.allArgs();
+      const lockCalls = setCalls.filter(args => args[0] === 'fastFeedbackOpening');
+      // first set to true, then released to false on error
+      expect(lockCalls).toEqual([
+        ['fastFeedbackOpening', true],
+        ['fastFeedbackOpening', false],
+      ]);
+    }));
+  });
+
+  describe('when testing submit()', () => {
+    it('should call graphQLMutate with answers and params', () => {
+      const answers = [{ questionId: 7, choiceId: 1 }];
+      const params = { teamId: 100, contextId: 200 };
+      apolloSpy.graphQLMutate.and.returnValue(of({ data: { submitPulseCheck: true } }));
 
-      service.pullFastFeedback().subscribe(res => {
-        expect(notificationSpy.modal).not.toHaveBeenCalled();
+      service.submit(answers, params).subscribe(() => {
+        expect(apolloSpy.graphQLMutate).toHaveBeenCalledTimes(1);
       });
     });
   });
-
-
 });
diff --git a/projects/v3/src/app/services/fast-feedback.service.ts b/projects/v3/src/app/services/fast-feedback.service.ts
