diff --git a/.gitignore b/.gitignore index 9248afa01..4b7fecd15 100644 --- 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 new file mode 100644 index 000000000..af3e25f3b --- /dev/null +++ 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 index 180287823..d31da9165 100644 --- 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; 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; + 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 index b1748390b..a83c51434 100644 --- 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 index 8b96b7466..f32ea8e40 100644 --- 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 @@ - + + diff --git a/projects/v3/src/app/services/fast-feedback.service.spec.ts b/projects/v3/src/app/services/fast-feedback.service.spec.ts index e5afeae6f..2d70f40ef 100644 --- 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; + let apolloSpy: jasmine.SpyObj; let notificationSpy: jasmine.SpyObj; let storageSpy: jasmine.SpyObj; - 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; + apolloSpy = TestBed.inject(ApolloService) as jasmine.SpyObj; notificationSpy = TestBed.inject(NotificationsService) as jasmine.SpyObj; storageSpy = TestBed.inject(BrowserStorageService) as jasmine.SpyObj; }); @@ -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 index 97247c285..08eb56d8e 100644 --- a/projects/v3/src/app/services/fast-feedback.service.ts +++ b/projects/v3/src/app/services/fast-feedback.service.ts @@ -2,8 +2,8 @@ import { Injectable } from '@angular/core'; import { NotificationsService } from './notifications.service'; import { BrowserStorageService } from '@v3/services/storage.service'; import { UtilsService } from '@v3/services/utils.service'; -import { of, from, Observable } from 'rxjs'; -import { switchMap, retry, finalize } from 'rxjs/operators'; +import { of, Observable } from 'rxjs'; +import { switchMap, retry } from 'rxjs/operators'; import { environment } from '@v3/environments/environment'; import { DemoService } from './demo.service'; import { ApolloService } from './apollo.service'; @@ -93,6 +93,10 @@ export class FastFeedbackService { closable: false, }): Observable { return this._getFastFeedback(options.skipChecking, options.type).pipe( + retry({ + count: 3, + delay: 1000 + }), switchMap((res) => { try { // don't open it again if there's one opening @@ -120,25 +124,26 @@ export class FastFeedbackService { questions?.length > 0 && !fastFeedbackIsOpened ) { - // add a flag to indicate that a fast feedback pop up is opening + // set a flag to indicate a fast feedback modal is currently opening to prevent duplicates. + // the lock stays true until FastFeedbackComponent.dismiss() releases it. this.storage.set("fastFeedbackOpening", true); - return from( - this.notificationsService.fastFeedbackModal( - { - questions, - meta, - }, - { - closable: options.closable, - modalOnly: options.modalOnly, - } - ) - ).pipe( - finalize(() => { - this.storage.set("fastFeedbackOpening", false); - }) - ); + // fire-and-forget: addModal() resolves immediately (before modal is dismissed), + // so we must NOT use from(promise).pipe(finalize(...)) — that would release the + // lock within 1 ms, defeating the duplicate-open guard. + this.notificationsService.fastFeedbackModal( + { + questions, + meta, + }, + { + closable: options.closable, + modalOnly: options.modalOnly, + } + ).catch(() => { + // release the lock only if the modal fails to open + this.storage.set("fastFeedbackOpening", false); + }); } return of(res); } catch (error) { @@ -151,10 +156,6 @@ export class FastFeedbackService { }); } }), - retry({ - count: 3, - delay: 1000 - }) ); }