Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,4 @@ www/*
.env
projects/v3/src/environments/environment.ts
projects/v3/src/environments/environment.prod.ts
output/
83 changes: 83 additions & 0 deletions docs/fixes/CORE-8002-pulsecheck-workflow.md
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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()', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down
191 changes: 111 additions & 80 deletions projects/v3/src/app/services/fast-feedback.service.spec.ts
Original file line number Diff line number Diff line change
@@ -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({
Expand All @@ -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>;
});
Expand All @@ -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);
});
});
});


});
Loading
Loading