From 1ddf93621c4145db63c4a8c7581621546eb871ef Mon Sep 17 00:00:00 2001 From: trtshen Date: Wed, 10 Sep 2025 12:16:22 +0800 Subject: [PATCH 01/10] [CORE-7940] pulsecheck race-condition --- .../fast-feedback/fast-feedback.component.ts | 5 +- .../src/app/components/img/img.component.ts | 4 +- .../activity-desktop/activity-desktop.page.ts | 12 +-- projects/v3/src/app/pages/home/home.page.ts | 12 --- .../src/app/services/fast-feedback.service.ts | 90 +++++++++++++++++-- projects/v3/src/app/services/home.service.ts | 1 + projects/v3/src/app/services/modal.service.ts | 31 ++++++- .../src/app/services/notifications.service.ts | 17 ++-- 8 files changed, 137 insertions(+), 35 deletions(-) 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 5214da4e7..ab82fe620 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 @@ -30,6 +30,7 @@ export class FastFeedbackComponent implements OnInit { loading = false; submissionCompleted: boolean; isMobile: boolean; + pulseCheckId: string; // pagination properties currentPage = 0; @@ -72,6 +73,8 @@ export class FastFeedbackComponent implements OnInit { this.totalPages = Math.ceil(this.questions.length / this.questionsPerPage); this.showPagination = this.totalPages > 1; + this.pulseCheckId = this.navParams.get('modal')?.componentProps?.pulseCheckId; + // Determine pulse check type based on question IDs this.pulseCheckType = this.determinePulseCheckType(); } @@ -227,7 +230,7 @@ export class FastFeedbackComponent implements OnInit { let submissionResult; try { submissionResult = await firstValueFrom(this.fastFeedbackService - .submit(answers, params)); + .submit(answers, params, this.pulseCheckId)); // Check if question 7's answer is 0 const question7Answer = formData['7']; // hardcoded question id 7 (1st fast feedback question) diff --git a/projects/v3/src/app/components/img/img.component.ts b/projects/v3/src/app/components/img/img.component.ts index e6bc39bba..2e3e82d8d 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, SimpleChanges, OnChanges } 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..c246a26d7 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 @@ -396,25 +396,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/home/home.page.ts b/projects/v3/src/app/pages/home/home.page.ts index da1a3db30..23a35a46f 100644 --- a/projects/v3/src/app/pages/home/home.page.ts +++ b/projects/v3/src/app/pages/home/home.page.ts @@ -189,18 +189,6 @@ export class HomePage implements OnInit, OnDestroy, AfterViewChecked { this.unsubscribe$.complete(); } - /** - * @name openPulseCheck - * @description This method pulls the fast feedback service (with type 'skills') to open the pulse check modal. - */ - openPulseCheck() { - this.fastFeedbackService.pullFastFeedback({ - closable: true, - skipChecking: true, - type: 'skills' - }).pipe(first()).subscribe(); - } - async updateDashboard() { await this.sharedService.refreshJWT(); // refresh JWT token [CORE-6083] this.experience = this.storageService.get("experience"); diff --git a/projects/v3/src/app/services/fast-feedback.service.ts b/projects/v3/src/app/services/fast-feedback.service.ts index 97247c285..eb3633bce 100644 --- a/projects/v3/src/app/services/fast-feedback.service.ts +++ b/projects/v3/src/app/services/fast-feedback.service.ts @@ -3,7 +3,7 @@ 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 { switchMap, retry, finalize, tap } from 'rxjs/operators'; import { environment } from '@v3/environments/environment'; import { DemoService } from './demo.service'; import { ApolloService } from './apollo.service'; @@ -13,6 +13,10 @@ import { ApiResponse } from '../models/api.model'; providedIn: 'root' }) export class FastFeedbackService { + private readonly SUBMISSION_COOLDOWN = 10 * 1000; // 10 seconds cooldown + + private currentPulseCheckId: string = null; // temporary store active pulse check ID + constructor( private notificationsService: NotificationsService, private storage: BrowserStorageService, @@ -114,13 +118,24 @@ export class FastFeedbackService { return of(res); } + // generate ID for this pulse check modal + const pulseCheckId = this.generatePulseCheckId(questions, meta); + + // skip showing the modal if this pulse check was recently viewed + submitted + if (this.isPulseCheckSubmitted(pulseCheckId) && !options.skipChecking) { + return of(res); + } + + // temporarily store the current pulse check ID after make sure it hasn't been submitted yet + this.currentPulseCheckId = pulseCheckId; + // popup instant feedback view if question quantity found > 0 if ( !this.utils.isEmpty(res.data) && 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 this.storage.set("fastFeedbackOpening", true); return from( @@ -128,6 +143,7 @@ export class FastFeedbackService { { questions, meta, + pulseCheckId, }, { closable: options.closable, @@ -143,7 +159,7 @@ export class FastFeedbackService { return of(res); } catch (error) { console.error("Error in switchMap:", error); - // Return a fallback observable to allow the consumer to continue working + // fail gracefully to avoid blocking user's flow return of({ error: true, message: "An error occurred while processing fast feedback.", @@ -154,7 +170,7 @@ export class FastFeedbackService { retry({ count: 3, delay: 1000 - }) + }), ); } @@ -162,13 +178,17 @@ export class FastFeedbackService { teamId?: number; targetUserId?: number; contextId?: number; - }): Observable { + }, + pulseCheckId?: string + ): Observable { if (environment.demo) { /* eslint-disable no-console */ console.log('data', answers, 'params', params); return this.demo.normalResponse() as Observable; } + const submittedId = pulseCheckId || this.currentPulseCheckId; // fallback to temporary ID if not provided + return this.apolloService.graphQLMutate( `mutation submitPulseCheck($teamId: Int, $targetUserId: Int, $contextId: Int, $answers: [PulseCheckAnswerInput]) { submitPulseCheck(teamId: $teamId, targetUserId: $targetUserId, contextId: $contextId, answers: $answers) @@ -177,6 +197,66 @@ export class FastFeedbackService { ...params, answers, }, + ).pipe( + tap(result => { + if (result.data?.submitPulseCheck && submittedId) { + this.recordPulseCheckSubmission(submittedId); + } + }) ); } + + /** + * generates a unique id for a pulse check based on its content + */ + private generatePulseCheckId(questions: any[], meta: any): string { + if (!questions?.length || !meta) { + return null; + } + + const questionIds = questions.map(q => q.id).sort().join(','); + return `${questionIds}_${meta.teamId}_${meta.contextId || 0}`; // eg. "1,2,3_45_0" + } + + /** + * checks if this specific pulse check was recently submitted + */ + private isPulseCheckSubmitted(pulseCheckId: string): boolean { + if (!pulseCheckId) { + return false; + } + + const submittedChecks = this.storage.get('submittedPulseChecks') || {}; + const submission = submittedChecks[pulseCheckId]; + + if (!submission) { + return false; + } + + const now = Date.now(); + return (now - submission) < this.SUBMISSION_COOLDOWN; + } + + /** + * records a specific pulse check as submitted + */ + private recordPulseCheckSubmission(pulseCheckId: string): void { + if (!pulseCheckId) { + return; + } + + // Record specific pulse check submission + const submittedChecks = this.storage.get('submittedPulseChecks') || {}; + submittedChecks[pulseCheckId] = Date.now(); + + // Clean up old submissions (older than cooldown period) + const now = Date.now(); + Object.keys(submittedChecks).forEach(id => { + if (now - submittedChecks[id] > this.SUBMISSION_COOLDOWN) { + delete submittedChecks[id]; + } + }); + + this.storage.set('submittedPulseChecks', submittedChecks); + } } diff --git a/projects/v3/src/app/services/home.service.ts b/projects/v3/src/app/services/home.service.ts index e3324c17b..32cf8fea9 100644 --- a/projects/v3/src/app/services/home.service.ts +++ b/projects/v3/src/app/services/home.service.ts @@ -366,6 +366,7 @@ export class HomeService { ); } + // update skill & progress survey matrix getPulseCheckSkills(): Observable> { diff --git a/projects/v3/src/app/services/modal.service.ts b/projects/v3/src/app/services/modal.service.ts index 117396177..eb1c89721 100644 --- a/projects/v3/src/app/services/modal.service.ts +++ b/projects/v3/src/app/services/modal.service.ts @@ -7,11 +7,32 @@ import { ModalController } from '@ionic/angular'; export class ModalService { private modalQueue: any[] = []; private isShowingModal = false; - + private activeModalIds: Set = new Set(); constructor(private modalController: ModalController) { } - async addModal(modalConfig: any, callback?: Function) { - this.modalQueue.push({ modalConfig, callback }); + /** + * Adds a modal to the queue to be displayed + * @param modalConfig The configuration for the modal + * @param callback Optional callback to execute after modal is dismissed + * @param modalId Optional unique identifier to prevent duplicate modals + * @returns Promise that resolves once the modal is added to queue + */ + async addModal(modalConfig: any, callback?: Function, modalId?: string): Promise { + // check if the modalId already in queue or being shown + if (modalId && this.activeModalIds.has(modalId)) { + return; + } + + if (modalId) { + this.activeModalIds.add(modalId); + } + + this.modalQueue.push({ + modalConfig, + callback, + modalId + }); + this.showNextModal(); } @@ -26,6 +47,10 @@ export class ModalService { this.isShowingModal = true; modal.onDidDismiss().then(() => { + if (modalInfo.modalId) { + this.activeModalIds.delete(modalInfo.modalId); + } + if (modalInfo.callback) { modalInfo.callback(); } diff --git a/projects/v3/src/app/services/notifications.service.ts b/projects/v3/src/app/services/notifications.service.ts index 6b8a5a6b2..cfe35e4cf 100644 --- a/projects/v3/src/app/services/notifications.service.ts +++ b/projects/v3/src/app/services/notifications.service.ts @@ -203,16 +203,16 @@ export class NotificationsService { return modal; } - async modal(component, componentProps, options?, event?): Promise { - return this.modalOnly(component, componentProps, options, event); + async modal(component, componentProps, options?, event?, modalId?: string): Promise { + return this.modalOnly(component, componentProps, options, event, modalId); } - async modalOnly(component, componentProps, options?, event?): Promise { + async modalOnly(component, componentProps, options?, event?, modalId?: string): Promise { const modalConfig = this.modalConfig( { component, componentProps }, options ); - return this.modalService.addModal(modalConfig, event); + return this.modalService.addModal(modalConfig, event, modalId); } /** @@ -449,6 +449,7 @@ export class NotificationsService { props: { questions?: Question[]; meta?: Meta | Object; + pulseCheckId?: string; }, options: { closable?: boolean; @@ -463,11 +464,15 @@ export class NotificationsService { showBackdrop: false, ...options }; + + // use pulseCheckId to identify each modal instance to prevent duplicate + const modalId = props.pulseCheckId ? `pulse-check-${props.pulseCheckId}` : null; + if (options.modalOnly) { - return this.modalOnly(FastFeedbackComponent, props, modalConfig); + return this.modalOnly(FastFeedbackComponent, props, modalConfig, null, modalId); } - return this.modal(FastFeedbackComponent, props, modalConfig); + return this.modal(FastFeedbackComponent, props, modalConfig, null, modalId); } getTodoItems(): Observable { From 9f91f2796e167ebe534381f8b58e05f9b19e180b Mon Sep 17 00:00:00 2001 From: trtshen Date: Fri, 26 Sep 2025 16:09:01 +0800 Subject: [PATCH 02/10] [CORE-8026] add setTimer to delay validation check before form is populated --- docs/assessment-btndisabled-flow.md | 163 +++++++++++ docs/assessment-flow.md | 253 +++++++++++++++--- .../assessment/assessment.component.ts | 80 +++++- 3 files changed, 448 insertions(+), 48 deletions(-) create mode 100644 docs/assessment-btndisabled-flow.md diff --git a/docs/assessment-btndisabled-flow.md b/docs/assessment-btndisabled-flow.md new file mode 100644 index 000000000..dde8195d4 --- /dev/null +++ b/docs/assessment-btndisabled-flow.md @@ -0,0 +1,163 @@ +═══════════════════════════════════════════════════════════════════════════════════════ + btnDisabled$ BehaviorSubject Flow Diagram +═══════════════════════════════════════════════════════════════════════════════════════ + +┌─────────────────────────────────────┐ +│ activity-desktop.page.ts │ +│ (Parent Component) │ +└─────────────────────────────────────┘ + │ + │ Creates & Passes btnDisabled$ + │ as @Input to assessment.component + ▼ +┌─────────────────────────────────────┐ +│ assessment.component.ts │ +│ (Child Component) │ +│ │ +│ @Input() btnDisabled$: │ +│ BehaviorSubject │ +└─────────────────────────────────────┘ + +═══════════════════════════════════════════════════════════════════════════════════════ + TRIGGER POINTS IN assessment.component.ts +═══════════════════════════════════════════════════════════════════════════════════════ + +1. ngOnChanges() - Component Lifecycle + └── btnDisabled$.next(false) ──────────► RESET on assessment change + +2. _populateQuestionsForm() - Form Setup + ├── If no questions exist: + │ └── btnDisabled$.next(true) ───────► DISABLE (empty form) + │ + └── questionsForm.valueChanges.subscribe() + └── setSubmissionDisabled() ───────► CHECK & UPDATE based on validation + +3. _handleSubmissionData() - Submission State Handler + └── If submission.isLocked: + └── btnDisabled$.next(true) ───────► DISABLE (locked by another user) + +4. _handleReviewData() - Review State Handler + └── If isPendingReview && review.status === 'in progress': + └── btnDisabled$.next(false) ──────► ENABLE for review + +5. continueToNextTask() - Submit Action + └── If _btnAction === 'submit': + └── btnDisabled$.next(true) ───────► DISABLE during submission + +6. _submitAnswer() - Answer Submission + └── If required questions missing: + └── btnDisabled$.next(false) ──────► RE-ENABLE after validation fail + +7. resubmit() - Resubmission Flow + ├── Start: btnDisabled$.next(true) ────► DISABLE during resubmit + └── End: btnDisabled$.next(false) ─────► RE-ENABLE after completion + +8. setSubmissionDisabled() - Main Validation Logic + ├── Only runs if (doAssessment || isPendingReview) + ├── If form invalid & not disabled: + │ └── btnDisabled$.next(true) ───────► DISABLE + └── If form valid & disabled: + └── btnDisabled$.next(false) ──────► ENABLE + +9. _prefillForm() - Form Population + ├── After populating form with answers + ├── questionsForm.updateValueAndValidity() + └── If edit mode (doAssessment || isPendingReview): + └── setSubmissionDisabled() ───────► CHECK & UPDATE validation + └── If read-only mode: + └── btnDisabled$.next(false) ──────► ENSURE enabled + +10. Page Navigation Methods + ├── goToPage() + ├── nextPage() + └── prevPage() + └── setSubmissionDisabled() ──────► CHECK & UPDATE for new page + +═══════════════════════════════════════════════════════════════════════════════════════ + TRIGGER CONDITIONS SUMMARY +═══════════════════════════════════════════════════════════════════════════════════════ + +DISABLE CONDITIONS (btnDisabled$.next(true)): +├── No questions in assessment +├── Assessment is locked by another user +├── Form is invalid (required fields empty) +├── During submission process +└── During resubmit process + +ENABLE CONDITIONS (btnDisabled$.next(false)): +├── Assessment changes (reset) +├── Form becomes valid +├── Review in progress +├── After failed validation alert +├── After resubmit completion +└── Read-only mode (not doAssessment && not isPendingReview) + +═══════════════════════════════════════════════════════════════════════════════════════ + PROBLEM SCENARIO +═══════════════════════════════════════════════════════════════════════════════════════ + +User Flow - Original Issue (RESOLVED): +1. User visits Assessment A (has required fields) + └── Form invalid → btnDisabled$.next(true) ✓ + +2. User navigates to Assessment B via activity-desktop + └── ngOnChanges() → btnDisabled$.next(false) ✓ + └── _populateQuestionsForm() → questionsForm created + └── _populateFormWithAnswers() → form populated + └── setSubmissionDisabled() → checks validation + └── BUT: Timing issue - form may not be fully populated + └── Result: btnDisabled$ may remain false even if invalid + +3. RESOLVED: State synchronization fixed + └── _prefillForm() now properly checks validation after form population + +═══════════════════════════════════════════════════════════════════════════════════════ + SOLUTION IMPLEMENTATION (COMPLETED) +═══════════════════════════════════════════════════════════════════════════════════════ + +IMPLEMENTED FIXES: +1. ✅ Reset state in ngOnChanges when assessment changes + └── btnDisabled$.next(false) in ngOnChanges() + +2. ✅ Proper validation after form population in _prefillForm() + ├── questionsForm.updateValueAndValidity() + ├── Edit mode: setSubmissionDisabled() checks validation + └── Read-only mode: btnDisabled$.next(false) ensures enabled + +3. ✅ Check validation when changing pages + ├── prevPage() → setSubmissionDisabled() + ├── nextPage() → setSubmissionDisabled() + └── goToPage() → setSubmissionDisabled() + +4. ✅ Apply validation rules only when in edit mode + └── setSubmissionDisabled() has guard: (!doAssessment && !isPendingReview) + +5. ✅ Replaced _populateFormWithAnswers() with _prefillForm() + └── Better state management and validation synchronization + +RESULT: btnDisabled$ now accurately reflects form state at all times +═══════════════════════════════════════════════════════════════════════════════════════ + +═══════════════════════════════════════════════════════════════════════════════════════ + KEY LEARNINGS +═══════════════════════════════════════════════════════════════════════════════════════ + +1. BEHAVIORSUBJECT STATE PERSISTENCE + └── BehaviorSubject remembers last value across component input changes + └── Must explicitly reset when navigating between assessments + +2. FORM VALIDATION TIMING + └── Validation must happen AFTER form population is complete + └── updateValueAndValidity() is crucial for proper validation state + +3. SEPARATION OF CONCERNS + └── setSubmissionDisabled() handles validation-based enabling/disabling + └── _prefillForm() handles initial state setup after population + └── Each method has clear responsibility boundaries + +4. EDIT VS READ-ONLY MODES + └── Only apply validation rules when user can edit + └── Read-only mode should always have enabled button for navigation + └── Guard clauses prevent unnecessary state changes + +═══════════════════════════════════════════════════════════════════════════════════════ \ No newline at end of file diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index a031fd3cb..063ff364c 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -12,10 +12,24 @@ The assessment system follows a hierarchical component structure with clear sepa Activity Pages (Desktop/Mobile) ↓ Assessment Component (Central Hub) + ↓ (with Pagination enabled) +Page Indicators ←→ Question Groups (Split into Pages) ←→ Navigation Controls ↓ Question Components (Text, File, Multiple Choice, etc.) ↓ -Bottom Action Bar (Submit/Continue Button) +Bottom Action Bar (Submit/Continue Button + Pagination Controls) +``` + +### Pagination Flow +When pagination is enabled (`environment.featureToggles.assessmentPagination = true` - environment variable file): + +``` +1. Assessment loads → splitGroupsByQuestionCount() +2. Groups divided into pages (≤8 questions per page) +3. Page indicators show completion status +4. Users navigate: Prev/Next buttons or click page indicators +5. Form validation tracks completion per page +6. Submit button integrates with pagination controls ``` ## Core Components @@ -122,33 +136,49 @@ questionsForm: FormGroup = new FormGroup({}); #### Form Population Logic +The form population has been refactored to ensure proper timing and validation state management. + **Assessment Answers (`this.action === 'assessment'`):** ```typescript -// Populate with submission answers -if (this.submission?.answers) { - Object.keys(this.submission.answers).forEach(questionId => { - const control = this.questionsForm.get('q-' + questionId); - if (control && this.submission.answers[questionId]?.answer !== undefined) { - control.setValue(this.submission.answers[questionId].answer, { emitEvent: false }); - } - }); -} -``` +private _prefillForm(): void { + // populate form with submission answers (for assessment action) + if (this.submission?.answers && this.action === 'assessment') { + Object.keys(this.submission.answers).forEach(questionId => { + const controlName = 'q-' + questionId; + const control = this.questionsForm.get(controlName); + if (control && this.submission.answers[questionId]?.answer !== undefined) { + control.setValue(this.submission.answers[questionId].answer, { emitEvent: false }); + } + }); + } -**Review Answers (`this.action === 'review'`):** -```typescript -// Populate with review answers (answer + comment structure) -if (this.review?.answers) { - Object.keys(this.review.answers).forEach(questionId => { - const control = this.questionsForm.get('q-' + questionId); - if (control && this.review.answers[questionId]) { - const reviewAnswer = { - answer: this.review.answers[questionId].answer, - comment: this.review.answers[questionId].comment - }; - control.setValue(reviewAnswer, { emitEvent: false }); - } - }); + // populate form with review answers (for review action) + if (this.review?.answers && this.action === 'review') { + Object.keys(this.review.answers).forEach(questionId => { + const controlName = 'q-' + questionId; + const control = this.questionsForm.get(controlName); + if (control && this.review.answers[questionId]) { + const reviewAnswer = { + answer: this.review.answers[questionId].answer, + comment: this.review.answers[questionId].comment, + file: this.review.answers[questionId].file || null, + }; + control.setValue(reviewAnswer, { emitEvent: false }); + } + }); + } + + // revalidate form after setting values + this.questionsForm.updateValueAndValidity(); + + // check validation state and update button accordingly + if (this.doAssessment || this.isPendingReview) { + // in edit mode, check form validation + this.setSubmissionDisabled(); + } else { + // in read-only mode, ensure button is enabled + this.btnDisabled$.next(false); + } } ``` @@ -347,16 +377,25 @@ private _isRequired(question: Question): boolean { **Button State Management:** ```typescript -this.questionsForm.valueChanges.pipe( - debounceTime(300), - takeUntil(this.unsubscribe$) -).subscribe(() => { - // Update button disabled state based on form validity - this.btnDisabled$.next(this.questionsForm.invalid); +// Delayed subscription to avoid race conditions during initialization +setTimeout(() => { + this.questionsForm.valueChanges.pipe( + takeUntil(this.unsubscribe$), + debounceTime(300), + ).subscribe(() => { + this.initializePageCompletion(); + this.setSubmissionDisabled(); + }); +}, 300); + +setSubmissionDisabled() { + // only enforce form validation when user can actually edit + if (!this.doAssessment && !this.isPendingReview) { + return; + } - // Update page completion tracking for pagination - this.initializePageCompletion(); -}); + this.btnDisabled$.next(this.questionsForm.invalid); +} ``` ### Validation Flow for Required File Questions @@ -417,16 +456,124 @@ goToPage(i: number) // Jump to specific page with validation ``` ### Completion Tracking + +The completion tracking system has been improved to handle proper initialization timing and avoid the "incompleted" class showing incorrectly on first load. + ```typescript +ngOnChanges(changes: SimpleChanges): void { + if (!this.assessment) { + return; + } + + this._initialise(); + + if (changes.assessment || changes.submission || changes.review) { + // reset button state when assessment changes + this.btnDisabled$.next(false); + this.pageRequiredCompletion = []; + + this._handleSubmissionData(); + this._populateQuestionsForm(); + this._handleReviewData(); + this._prefillForm(); + } + + // split by question count every time assessment changes - only if pagination is enabled + if (this.isPaginationEnabled) { + this.pagesGroups = this.splitGroupsByQuestionCount(); + this.pageIndex = 0; + + // initialize page completion after form is fully set up + // use delay to ensure form values are populated + setTimeout(() => { + this.initializePageCompletion(); + }, 200); + } else { + // Reset pagination data when disabled + this.pagesGroups = []; + this.pageIndex = 0; + } + + // scroll to the active page into view after rendering + setTimeout(() => this.scrollActivePageIntoView(), 250); +} + initializePageCompletion() { - // Updates status array for required questions - this.pageRequiredCompletion = this.pagesGroups.map(pageGroups => { - const questions = this.getAllQuestionsForPage(pageIndex); - return this.areAllRequiredQuestionsAnswered(questions); + if (!this.isPaginationEnabled) return; + + this.pageRequiredCompletion = new Array(this.pageCount).fill(true); + + this.pages.forEach((page, index) => { + const pageQuestions = this.getAllQuestionsForPage(index); + this.pageRequiredCompletion[index] = this.areAllRequiredQuestionsAnswered(pageQuestions); }); + + // trigger change detection to update the view + this.cdr.detectChanges(); + + // Update the scroll position when page completion status changes + setTimeout(() => this.scrollActivePageIntoView(), 100); } ``` +**Key Improvements for First Load Issue:** +1. **Timing Fix**: `initializePageCompletion()` is now called in `ngOnChanges()` after pagination setup with a 200ms delay +2. **Change Detection**: Added `this.cdr.detectChanges()` to ensure the view updates when completion status changes +3. **Form Population Order**: Form values are populated via `_prefillForm()` before completion tracking runs +4. **Race Condition Prevention**: Delayed form valueChanges subscription to avoid interference during initialization + +## Pagination Issue Fixes + +### Problem: "Incompleted" Class on First Load + +**Issue Description:** +Page indicators showed up with the "incompleted" class on first load of assessments, even when questions were already answered. This occurred due to a timing mismatch between form population and completion tracking initialization. + +**Root Cause:** +The `initializePageCompletion()` method was being called before form values were fully populated, causing `areAllRequiredQuestionsAnswered()` to return false for completed questions. + +**Solution Implemented:** + +1. **Moved completion initialization to proper lifecycle hook:** + ```typescript + // In ngOnChanges(), after pagination setup + setTimeout(() => { + this.initializePageCompletion(); + }, 200); + ``` + +2. **Added change detection trigger:** + ```typescript + initializePageCompletion() { + // ... completion logic ... + this.cdr.detectChanges(); // Ensure view updates + } + ``` + +3. **Separated form population logic:** + ```typescript + private _prefillForm(): void { + // Form population with proper validation state management + // Called before completion tracking + } + ``` + +4. **Delayed form valueChanges subscription:** + ```typescript + setTimeout(() => { + this.questionsForm.valueChanges.pipe( + takeUntil(this.unsubscribe$), + debounceTime(300), + ).subscribe(() => { + this.initializePageCompletion(); + this.setSubmissionDisabled(); + }); + }, 300); + ``` + +**Result:** +Page indicators now correctly show completion status on first load, with proper visual feedback for answered and unanswered required questions. + ## Error Handling ### Validation Errors @@ -475,6 +622,26 @@ shareReplay(1) // Cache service responses - Assessment data fetched when needed - Pagination reduces DOM complexity +## Troubleshooting + +### Common Pagination Issues + +1. **Page indicators show as incomplete on first load:** + - **Cause**: `initializePageCompletion()` called before form values are set + - **Solution**: Ensure proper timing in `ngOnChanges()` with delays + +2. **Form validation not working correctly:** + - **Cause**: Race condition between form population and validation setup + - **Solution**: Use `_prefillForm()` method with proper sequencing + +3. **Change detection not triggering:** + - **Cause**: OnPush change detection strategy requires manual triggering + - **Solution**: Call `this.cdr.detectChanges()` after completion updates + +4. **Button state incorrect on load:** + - **Cause**: Button state set before form is properly initialized + - **Solution**: Use `setSubmissionDisabled()` method with proper conditions + ## Testing Considerations ### Unit Tests @@ -482,6 +649,7 @@ shareReplay(1) // Cache service responses - Test form validation logic - Verify button state changes - Component interaction testing +- Test pagination initialization timing ### Integration Tests - End-to-end assessment submission flow @@ -569,3 +737,12 @@ Key strengths: - Robust error handling and network resilience - Performance optimizations for large assessments - Comprehensive pagination system for long assessments +- **Improved initialization timing** to prevent incorrect "incompleted" status on first load +- **Proper change detection management** with OnPush strategy +- **Race condition prevention** through strategic delays and sequencing + +Recent improvements have specifically addressed timing issues that could cause pagination indicators to display incorrectly on first load, ensuring a more reliable and user-friendly assessment experience. + + +## References +- [Button Disabled State Flow](assessment-btndisabled-flow.md) - btnDisabled$ BehaviorSubject flow diagram across assessment component lifecycle \ No newline at end of file diff --git a/projects/v3/src/app/components/assessment/assessment.component.ts b/projects/v3/src/app/components/assessment/assessment.component.ts index 2fefacba0..873dafd47 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.ts +++ b/projects/v3/src/app/components/assessment/assessment.component.ts @@ -1,5 +1,5 @@ import { environment } from '@v3/environments/environment'; -import { Component, Input, Output, EventEmitter, OnChanges, OnDestroy, OnInit, QueryList, ViewChildren, ChangeDetectionStrategy, ViewChild, signal, ElementRef, SimpleChanges } from '@angular/core'; +import { Component, Input, Output, EventEmitter, OnChanges, OnDestroy, OnInit, QueryList, ViewChildren, ChangeDetectionStrategy, ViewChild, signal, ElementRef, SimpleChanges, ChangeDetectorRef } from '@angular/core'; import { Assessment, Submission, AssessmentReview, AssessmentSubmitParams, AssessmentService } from '@v3/services/assessment.service'; import { UtilsService } from '@v3/services/utils.service'; import { NotificationsService } from '@v3/services/notifications.service'; @@ -147,6 +147,7 @@ export class AssessmentComponent implements OnInit, OnChanges, OnDestroy { private sharedService: SharedService, private assessmentService: AssessmentService, private activityService: ActivityService, + private cdr: ChangeDetectorRef, ) { this.resubscribe$.pipe( takeUntil(this.unsubscribe$), @@ -415,6 +416,10 @@ Best regards`; if (this.isPaginationEnabled) { this.pagesGroups = this.splitGroupsByQuestionCount(); this.pageIndex = 0; + + setTimeout(() => { + this.initializePageCompletion(); + }, 200); } else { // Reset pagination data when disabled this.pagesGroups = []; @@ -424,7 +429,7 @@ Best regards`; this._populateFormWithAnswers(); // scroll to the active page into view after rendering - setTimeout(() => this.scrollActivePageIntoView(), 200); + setTimeout(() => this.scrollActivePageIntoView(), 250); } ngOnDestroy() { @@ -523,14 +528,21 @@ Best regards`; }); }); - this.questionsForm.valueChanges.pipe( - takeUntil(this.unsubscribe$), - debounceTime(300), - ).subscribe(() => { - this.initializePageCompletion(); - // this.btnDisabled$.next(this.questionsForm.invalid); - this.setSubmissionDisabled(); - }); + // when no questions in the assessment, disable the button + if (this.utils.isEmpty(this.questionsForm.getRawValue())) { + return this.btnDisabled$.next(true); + } + + // delay the subscription to avoid race conditions during initialization + setTimeout(() => { + this.questionsForm.valueChanges.pipe( + takeUntil(this.unsubscribe$), + debounceTime(300), + ).subscribe(() => { + this.initializePageCompletion(); + this.setSubmissionDisabled(); + }); + }, 300); } /** @@ -1051,6 +1063,8 @@ Best regards`; this.pageRequiredCompletion[index] = this.areAllRequiredQuestionsAnswered(pageQuestions); }); + this.cdr.detectChanges(); + // Update the scroll position when page completion status changes setTimeout(() => this.scrollActivePageIntoView(), 100); } @@ -1243,6 +1257,52 @@ Best regards`; }, 100); } + /** + * prefill form with answers and check validation state + * replaces the old _populateFormWithAnswers() method + */ + private _prefillForm(): void { + // populate form with submission answers (for assessment action) + if (this.submission?.answers && this.action === 'assessment') { + Object.keys(this.submission.answers).forEach(questionId => { + const controlName = 'q-' + questionId; + const control = this.questionsForm.get(controlName); + if (control && this.submission.answers[questionId]?.answer !== undefined) { + control.setValue(this.submission.answers[questionId].answer, { emitEvent: false }); + } + }); + } + + // populate form with review answers (for review action) + if (this.review?.answers && this.action === 'review') { + Object.keys(this.review.answers).forEach(questionId => { + const controlName = 'q-' + questionId; + const control = this.questionsForm.get(controlName); + if (control && this.review.answers[questionId]) { + const reviewAnswer = { + answer: this.review.answers[questionId].answer, + comment: this.review.answers[questionId].comment, + file: this.review.answers[questionId].file || null, + }; + control.setValue(reviewAnswer, { emitEvent: false }); + } + }); + } + + // revalidate form after setting values + this.questionsForm.updateValueAndValidity(); + + // check validation state and update button accordingly + if (this.doAssessment || this.isPendingReview) { + // in edit mode, check form validation + this.setSubmissionDisabled(); + } else { + // in read-only mode, ensure button is enabled + this.btnDisabled$.next(false); + } + } + + setSubmissionDisabled() { // only enforce form validation when user can actually edit if (!this.doAssessment && !this.isPendingReview) { From 1685b3de9905dde366d2ff63c5f6e8eb3d3ed731 Mon Sep 17 00:00:00 2001 From: trtshen Date: Thu, 25 Sep 2025 11:29:21 +0800 Subject: [PATCH 03/10] [CORE-7944] added exception for mobile review --- projects/v3/src/app/app.component.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/projects/v3/src/app/app.component.ts b/projects/v3/src/app/app.component.ts index e972ece91..0fc3d461b 100644 --- a/projects/v3/src/app/app.component.ts +++ b/projects/v3/src/app/app.component.ts @@ -29,7 +29,7 @@ export class AppComponent implements OnInit, OnDestroy { $unsubscribe = new Subject(); lastVisitedUrl: string; - // list of urls that should not be cached + // urls that should not be cached for last visited tracking noneCachedUrl = [ 'devtool', 'registration', @@ -40,6 +40,7 @@ export class AppComponent implements OnInit, OnDestroy { 'direct_login', 'do=secure', 'auth/secure', + 'assessment-mobile/review', 'undefined', ]; From d5c7057069ef0dbb509fffa596f63a62d94f553e Mon Sep 17 00:00:00 2001 From: trtshen Date: Mon, 29 Sep 2025 16:23:34 +0800 Subject: [PATCH 04/10] [CORE-8027] reduce prev & next gap --- .../app/components/assessment/assessment.component.html | 1 + .../app/components/assessment/assessment.component.scss | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/projects/v3/src/app/components/assessment/assessment.component.html b/projects/v3/src/app/components/assessment/assessment.component.html index 428ab710c..9c450b7ff 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.html +++ b/projects/v3/src/app/components/assessment/assessment.component.html @@ -375,6 +375,7 @@ [hasCustomContent]="isPaginationEnabled && pageCount > 1">
Date: Tue, 30 Sep 2025 12:06:53 +0800 Subject: [PATCH 05/10] [CORE-8028] increased pagination indicator size --- .../assessment/assessment.component.html | 13 +- .../assessment/assessment.component.scss | 126 ++++++++++++++---- .../assessment/assessment.component.ts | 14 +- 3 files changed, 126 insertions(+), 27 deletions(-) diff --git a/projects/v3/src/app/components/assessment/assessment.component.html b/projects/v3/src/app/components/assessment/assessment.component.html index 9c450b7ff..a513a62a9 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.html +++ b/projects/v3/src/app/components/assessment/assessment.component.html @@ -375,7 +375,7 @@ [hasCustomContent]="isPaginationEnabled && pageCount > 1">
- {{ i + 1 }} + +
+ {{ i + 1 }} + {{ getPageQuestionCount(page) }} +
+ + + diff --git a/projects/v3/src/app/components/assessment/assessment.component.scss b/projects/v3/src/app/components/assessment/assessment.component.scss index 069b33804..4e3194939 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.scss +++ b/projects/v3/src/app/components/assessment/assessment.component.scss @@ -254,10 +254,10 @@ ion-footer { flex-wrap: nowrap; align-items: center; justify-content: center; // center items by design - gap: 6px; + gap: 8px; overflow-x: visible; - max-width: 60%; - padding: 4px 0; + max-width: 65%; + padding: 6px 0; margin: 0 -5px; scrollbar-width: none; scroll-behavior: smooth; @@ -267,8 +267,8 @@ ion-footer { &.many-pages { overflow-x: auto; justify-content: flex-start; // Align to start when scrolling - mask-image: linear-gradient(to right, transparent, black 10px, black 90%, transparent); - -webkit-mask-image: linear-gradient(to right, transparent, black 10px, black 90%, transparent); + mask-image: linear-gradient(to right, transparent, black 15px, black 85%, transparent); + -webkit-mask-image: linear-gradient(to right, transparent, black 15px, black 85%, transparent); } } @@ -280,24 +280,27 @@ ion-footer { display: flex; align-items: center; justify-content: center; - min-width: 24px; - min-height: 24px; + min-width: 40px; + min-height: 40px; border-radius: 50%; background-color: #f5f5f5; cursor: pointer; position: relative; transition: all 0.2s ease; flex-shrink: 0; + border: 2px solid transparent; } .page-indicator:hover { background-color: #e0e0e0; + transform: scale(1.05); } .page-indicator.active { background-color: var(--ion-color-primary); - transform: scale(1.1); - box-shadow: 0 2px 4px rgba(0,0,0,0.2); + transform: scale(1.2); + box-shadow: 0 4px 8px rgba(0,0,0,0.3); + border-color: var(--ion-color-primary); } .page-indicator.active .page-number { @@ -307,51 +310,128 @@ ion-footer { .page-indicator.completed { background-color: #e8f5e9; - border: 1px solid #81c784; + border-color: #81c784; } .page-indicator.incompleted { background-color: rgba(255, 0, 0, 0.05); - border: 1px solid var(--ion-color-danger); + border-color: var(--ion-color-danger); } .page-indicator.incompleted .page-number { color: var(--ion-color-danger); } +.progress-ring { + position: relative; + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + width: 100%; + height: 100%; +} + +.progress-ring-svg { + width: 36px; + height: 36px; + transform: rotate(-90deg); + position: absolute; + top: 50%; + left: 50%; + transform: translate(-50%, -50%) rotate(-90deg); +} + +.progress-ring-bg { + fill: none; + stroke: #e0e0e0; + stroke-width: 2; +} + +.progress-ring-fill { + fill: none; + stroke: var(--ion-color-success); + stroke-width: 3; + stroke-linecap: round; + transition: stroke-dasharray 0.3s ease; +} + +.page-indicator.active .progress-ring-fill { + stroke: white; + stroke-width: 4; +} + +.page-indicator.incompleted .progress-ring-fill { + stroke: var(--ion-color-danger); +} + .page-number { - font-size: 12px; - font-weight: 500; + font-size: 14px; + font-weight: 600; color: #555555; + z-index: 2; + margin-bottom: 2px; +} + +.question-count { + font-size: 8px; + font-weight: 500; + color: #888888; + z-index: 2; + line-height: 1; +} + +.page-indicator.active .question-count { + color: rgba(255, 255, 255, 0.8); } .completion-icon { position: absolute; - font-size: 10px; - color: var(--ion-color-danger); - bottom: -3px; - right: -3px; + font-size: 12px; + color: var(--ion-color-success); + bottom: -2px; + right: -2px; background: white; border-radius: 50%; - z-index: 1; + z-index: 3; + width: 16px; + height: 16px; + display: flex; + align-items: center; + justify-content: center; +} + +.completion-icon:not(.completed) { + color: var(--ion-color-danger); } @media (max-width: 576px) { .pagination-container { - padding: 0;; + padding: 0; .page-indicators { - max-width: 50%; + max-width: 55%; + gap: 4px; .page-indicator { - min-width: 22px; - min-height: 22px; + min-width: 32px; + min-height: 32px; } } } .page-number { - font-size: 11px; + font-size: 12px; + } + + .question-count { + font-size: 7px; + } + + .completion-icon { + font-size: 10px; + width: 14px; + height: 14px; } .nav-button { diff --git a/projects/v3/src/app/components/assessment/assessment.component.ts b/projects/v3/src/app/components/assessment/assessment.component.ts index 873dafd47..1ed8fc24e 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.ts +++ b/projects/v3/src/app/components/assessment/assessment.component.ts @@ -20,7 +20,8 @@ import { ActivityService } from '@v3/app/services/activity.service'; import { FileInput, Question, SubmitActions } from '../types/assessment'; import { FileUploadComponent } from '../file-upload/file-upload.component'; -const MIN_SCROLLING_PAGES = 6; // minimum number of pages to show pagination scrolling +const MIN_SCROLLING_PAGES = 8; // minimum number of pages to show pagination scrolling +const MAX_QUESTIONS_PER_PAGE = 8; // maximum number of questions to display per paginated view (controls pagination granularity) /** * Assessment Component with optional pagination feature @@ -157,7 +158,7 @@ export class AssessmentComponent implements OnInit, OnChanges, OnDestroy { } - pageSize = 8; // number of questions per page + pageSize = MAX_QUESTIONS_PER_PAGE; // number of questions per page pageIndex = 0; // each entry is a page: an array of (partial) groups @@ -1327,4 +1328,13 @@ Best regards`; shouldShowRequiredIndicator(question: Question): boolean { return this._isRequired(question) && (this.doAssessment || this.isPendingReview); } + + /** + * Get the total number of questions on a specific page + * @param pageIndex - The index of the page + * @returns The number of questions on that page + */ + getPageQuestionCount(pageIndex: number): number { + return this.getAllQuestionsForPage(pageIndex).length; + } } From 11072c7dc45cbc2666af731332a1601c2241b49f Mon Sep 17 00:00:00 2001 From: trtshen Date: Tue, 30 Sep 2025 16:31:22 +0800 Subject: [PATCH 06/10] [CORE-8029] don't flick --- projects/v3/src/app/services/auth.service.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/projects/v3/src/app/services/auth.service.ts b/projects/v3/src/app/services/auth.service.ts index ded993515..56dd573f5 100644 --- a/projects/v3/src/app/services/auth.service.ts +++ b/projects/v3/src/app/services/auth.service.ts @@ -351,8 +351,6 @@ export class AuthService { * @param redirect Whether redirect the user to login page or not */ logout(navigationParams = {}, redirect: boolean | string[] = true) { - // use the config color - this.utils.changeThemeColor(this.storage.getConfig().colors); this.pusherService.unsubscribeChannels(); this.pusherService.disconnect(); const config = this.storage.getConfig(); From 61440dcd5f075432caa31973146af217de19a3d2 Mon Sep 17 00:00:00 2001 From: trtshen Date: Wed, 1 Oct 2025 08:55:01 +0800 Subject: [PATCH 07/10] [CORE-8028] removed question quanitiy indicator --- .../components/assessment/assessment.component.html | 1 - .../components/assessment/assessment.component.scss | 11 ----------- .../app/components/assessment/assessment.component.ts | 9 --------- 3 files changed, 21 deletions(-) diff --git a/projects/v3/src/app/components/assessment/assessment.component.html b/projects/v3/src/app/components/assessment/assessment.component.html index a513a62a9..98fd37d70 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.html +++ b/projects/v3/src/app/components/assessment/assessment.component.html @@ -399,7 +399,6 @@
{{ i + 1 }} - {{ getPageQuestionCount(page) }}
Date: Fri, 3 Oct 2025 15:27:02 +0800 Subject: [PATCH 08/10] [CORE-8031] readonly avoid required indicator --- docs/assessment-flow.md | 16 ++++++++++++++++ .../assessment/assessment.component.ts | 8 ++++++++ 2 files changed, 24 insertions(+) diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index 063ff364c..6a267aeaf 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -501,6 +501,16 @@ ngOnChanges(changes: SimpleChanges): void { initializePageCompletion() { if (!this.isPaginationEnabled) return; + // Only track completion status when user can actually edit the form + // In read-only mode (viewing feedback or completed submissions), completion tracking is not relevant + if (!this.doAssessment && !this.isPendingReview) { + // Set all pages as completed for read-only mode to avoid showing incomplete indicators + this.pageRequiredCompletion = new Array(this.pageCount).fill(true); + this.cdr.detectChanges(); + setTimeout(() => this.scrollActivePageIntoView(), 100); + return; + } + this.pageRequiredCompletion = new Array(this.pageCount).fill(true); this.pages.forEach((page, index) => { @@ -521,6 +531,7 @@ initializePageCompletion() { 2. **Change Detection**: Added `this.cdr.detectChanges()` to ensure the view updates when completion status changes 3. **Form Population Order**: Form values are populated via `_prefillForm()` before completion tracking runs 4. **Race Condition Prevention**: Delayed form valueChanges subscription to avoid interference during initialization +5. **Read-Only Mode Handling**: Completion tracking is disabled when users are viewing feedback or completed submissions (`!doAssessment && !isPendingReview`), showing all pages as completed instead ## Pagination Issue Fixes @@ -642,6 +653,11 @@ shareReplay(1) // Cache service responses - **Cause**: Button state set before form is properly initialized - **Solution**: Use `setSubmissionDisabled()` method with proper conditions +5. **Completion indicators showing in read-only mode:** + - **Cause**: Completion tracking running when user is viewing feedback/completed submissions + - **Solution**: Check `doAssessment` and `isPendingReview` flags before running completion logic + + ## Testing Considerations ### Unit Tests diff --git a/projects/v3/src/app/components/assessment/assessment.component.ts b/projects/v3/src/app/components/assessment/assessment.component.ts index c3d544cbe..7e7705773 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.ts +++ b/projects/v3/src/app/components/assessment/assessment.component.ts @@ -1057,6 +1057,14 @@ Best regards`; initializePageCompletion() { if (!this.isPaginationEnabled) return; + // read-only mode (viewing feedback or completed submissions), completion tracking is not relevant + if (!this.doAssessment && !this.isPendingReview) { + this.pageRequiredCompletion = new Array(this.pageCount).fill(true); + this.cdr.detectChanges(); + setTimeout(() => this.scrollActivePageIntoView(), 100); + return; + } + this.pageRequiredCompletion = new Array(this.pageCount).fill(true); this.pages.forEach((page, index) => { From 79fdaf7f68432696f92f9e62c8d80db9f349bcc4 Mon Sep 17 00:00:00 2001 From: trtshen Date: Thu, 26 Feb 2026 13:36:53 +0800 Subject: [PATCH 09/10] hide fab button temporarily in activity-desktop page - update condition to hide fab button for assessment type - add comment for clarity on temporary change --- .gitignore | 1 + .../src/app/pages/activity-desktop/activity-desktop.page.html | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) 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/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 @@ - + + From cb9981e7903cf89c8576787a66b02a5de993ee83 Mon Sep 17 00:00:00 2001 From: trtshen Date: Tue, 3 Mar 2026 13:40:02 +0800 Subject: [PATCH 10/10] [CORE-7940] fix: release fastFeedbackOpening lock on dismiss and destroy - update dismiss() to release lock in fast feedback component - add ngOnDestroy() to ensure lock is released on component destroy - modify tests to verify lock behavior during dismiss and destroy --- .../fast-feedback.component.spec.ts | 10 +- .../fast-feedback/fast-feedback.component.ts | 4 + .../services/fast-feedback.service.spec.ts | 191 ++++++++++-------- .../src/app/services/fast-feedback.service.ts | 39 ++-- 4 files changed, 144 insertions(+), 100 deletions(-) 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/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 17a2b1ce8..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'; @@ -124,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) {