From 41f5dc32d0c1622295c00ccf1555924d9390cdb2 Mon Sep 17 00:00:00 2001 From: trtshen Date: Thu, 9 Jul 2026 14:18:30 +0800 Subject: [PATCH 01/14] [CORE-8270] primary color for all --- docs/features/project-brief.md | 12 ++++++--- .../project-brief-modal.component.html | 10 ++++---- .../project-brief-modal.component.spec.ts | 25 +++++++++++++++++++ 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/docs/features/project-brief.md b/docs/features/project-brief.md index e6fcbb16f7..c4f7d9c11c 100644 --- a/docs/features/project-brief.md +++ b/docs/features/project-brief.md @@ -34,10 +34,11 @@ showProjectBrief() → opens ProjectBriefModalComponent - `project-brief-modal.component.scss` - Component-specific styles - `project-brief-modal.component.spec.ts` - Unit tests -**Input:** +**Modal property:** ```typescript -@Input() projectBrief: ProjectBrief = {}; +projectBrief: ProjectBrief = {}; ``` +Ionic sets this public property through `componentProps` when the modal is created. **Interface:** ```typescript @@ -62,6 +63,10 @@ interface ProjectBrief { - Professional Skills (as chips) - Deliverables +**Color treatment:** +- Modal section header icons and chips use the primary brand color. +- Do not use the secondary brand color for Technical Skills accents; customer secondary colors can be too light to remain visible on the light modal background. + **Empty Field Handling:** - All sections show "None specified" when the field is empty or undefined - Uses `hasValue()` for string fields and `hasItems()` for array fields @@ -181,7 +186,7 @@ Button placement - next to experience name: - Keyboard navigation with `(keydown.enter)` and `(keydown.space)` handlers - Modal has proper semantic structure with `
`, `
`, and heading hierarchy - Close button includes `aria-label="Close project brief"` -- Ion-chips for industry/skills are visually distinct with color coding +- Ion-chips for industry/skills use the primary brand color so labels and outlines remain visible when secondary branding is faint ## Sample Data @@ -219,6 +224,7 @@ After parsing: - Template renders title when provided - Template shows "None specified" for empty fields - Template renders chips for industry and skills +- Template keeps section accents on the primary brand color **HomePage tests (additions needed):** - Button visible when `projectBrief` is set diff --git a/projects/v3/src/app/components/project-brief-modal/project-brief-modal.component.html b/projects/v3/src/app/components/project-brief-modal/project-brief-modal.component.html index aea21a7a66..a1b0b029e3 100644 --- a/projects/v3/src/app/components/project-brief-modal/project-brief-modal.component.html +++ b/projects/v3/src/app/components/project-brief-modal/project-brief-modal.component.html @@ -59,13 +59,13 @@

{{ projectBrief.title }}

- + Technical Skills
{{ skill }} @@ -78,13 +78,13 @@

{{ projectBrief.title }}

- + Professional Skills
{{ skill }} @@ -97,7 +97,7 @@

{{ projectBrief.title }}

- + Deliverables
diff --git a/projects/v3/src/app/components/project-brief-modal/project-brief-modal.component.spec.ts b/projects/v3/src/app/components/project-brief-modal/project-brief-modal.component.spec.ts index 8fc024d6a3..03b5e95da9 100644 --- a/projects/v3/src/app/components/project-brief-modal/project-brief-modal.component.spec.ts +++ b/projects/v3/src/app/components/project-brief-modal/project-brief-modal.component.spec.ts @@ -117,5 +117,30 @@ describe('ProjectBriefModalComponent', () => { const chips = fixture.nativeElement.querySelectorAll('ion-chip'); expect(chips.length).toBe(4); }); + + it('should use the primary brand color for section accents', () => { + component.projectBrief = { + industry: ['Health'], + technicalSkills: ['Python'], + professionalSkills: ['Leadership'], + deliverables: 'Prototype' + }; + fixture.detectChanges(); + + const accentSelectors = [ + 'ion-icon[name="document-text-outline"]', + 'ion-icon[name="business-outline"]', + 'ion-icon[name="code-slash-outline"]', + 'ion-icon[name="people-outline"]', + 'ion-icon[name="checkbox-outline"]', + 'ion-chip' + ]; + + accentSelectors.forEach((selector) => { + fixture.nativeElement.querySelectorAll(selector).forEach((element: Element) => { + expect(element.getAttribute('color')).toBe('primary'); + }); + }); + }); }); }); From 08a4dc5c4a8e4b531acb85536029d655ea383b3d Mon Sep 17 00:00:00 2001 From: trtshen Date: Thu, 16 Jul 2026 15:03:24 +0800 Subject: [PATCH 02/14] [CORE-8277] reviewer-only improvement --- docs/assessment-flow.md | 14 +- docs/features/slider-rating-implementation.md | 1 + ...readonly-preview-multiple-selected-only.md | 70 +++-- .../assessment/assessment.component.html | 66 +++-- .../assessment/assessment.component.scss | 24 ++ .../assessment/assessment.component.spec.ts | 268 ++++++++++++++++++ .../assessment/assessment.component.ts | 106 ++++++- .../bottom-action-bar.component.html | 16 +- .../bottom-action-bar.component.scss | 8 +- .../bottom-action-bar.component.spec.ts | 61 ++++ .../bottom-action-bar.component.ts | 36 ++- .../file-upload/file-upload.component.html | 11 +- .../file-upload/file-upload.component.ts | 2 + .../multi-team-member-selector.component.html | 13 +- ...lti-team-member-selector.component.spec.ts | 46 +++ .../multi-team-member-selector.component.ts | 11 + .../multiple/multiple.component.html | 27 +- .../multiple/multiple.component.scss | 1 - .../multiple/multiple.component.spec.ts | 97 +++++-- .../components/multiple/multiple.component.ts | 17 ++ .../app/components/oneof/oneof.component.html | 20 +- .../components/oneof/oneof.component.spec.ts | 48 ++++ .../app/components/oneof/oneof.component.ts | 2 + .../components/slider/slider.component.html | 35 ++- .../components/slider/slider.component.scss | 1 - .../slider/slider.component.spec.ts | 63 ++++ .../app/components/slider/slider.component.ts | 12 + .../team-member-selector.component.html | 11 +- .../team-member-selector.component.spec.ts | 41 +++ .../team-member-selector.component.ts | 11 + .../app/components/text/text.component.html | 9 +- .../components/text/text.component.spec.ts | 45 ++- .../src/app/components/text/text.component.ts | 2 + 33 files changed, 1071 insertions(+), 124 deletions(-) diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index 4158a91111..ddb4876abd 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -300,17 +300,25 @@ All follow similar patterns with dual-purpose display for learner/reviewer conte ```html {{ text }} + [attr.aria-busy]="loading ? 'true' : 'false'"> + + {{ text }} + ``` **Button States:** - **Enabled**: Form is valid and user can submit -- **Disabled**: Form has validation errors or submission in progress +- **Disabled**: Form has validation errors or an action is already in progress +- **Loading**: For assessment/review submit actions, starts before the click event is emitted, keeps the disabled button visible with an inline spinner, and clears when `disabled$` emits `false` - **Dynamic Text**: Changes based on context (Submit, Continue, Mark as Read, etc.) +`disabled$` remains the source of truth for whether the action can be triggered. Loading is a distinct, opt-in visual state (`showLoadingOnClick`) so validation-disabled buttons do not incorrectly announce `aria-busy`, and non-submit actions retain their existing behavior. + +During manual submission, the parent page owns the terminal `disabled$ = false` transition. Intermediate assessment/review refetches may update displayed data and the last-saved message, but must not re-enable the action while the assessment component's submission guard is active. The parent clears the state only after the final refresh succeeds or the submission fails. + ## Data Flow Diagrams ### Assessment Submission Flow (Learner) diff --git a/docs/features/slider-rating-implementation.md b/docs/features/slider-rating-implementation.md index 93c74f5428..6dffb2ba14 100644 --- a/docs/features/slider-rating-implementation.md +++ b/docs/features/slider-rating-implementation.md @@ -261,6 +261,7 @@ pinFormatter = (value: number): string => { margin-top: 16px; .label { + // Uses the shared ion-chip.label sizing and typography. &.orange { /* Learner answer styling */ } &.yellow { /* Expert answer styling */ } } diff --git a/docs/fixes/readonly-preview-multiple-selected-only.md b/docs/fixes/readonly-preview-multiple-selected-only.md index 210154fd70..b5b1de452e 100644 --- a/docs/fixes/readonly-preview-multiple-selected-only.md +++ b/docs/fixes/readonly-preview-multiple-selected-only.md @@ -2,36 +2,70 @@ status: stable authority: historical scope: frontend -last_reviewed: 2026-05-21 -supersedes: none +last_reviewed: 2026-07-16 +supersedes: CORE-8277 all-choice checkbox feedback presentation --- -# readonly preview: show selected answers only (multiple & oneof) +# readonly selected answers and reviewer feedback context ## rule -in preview and other readonly display-only states (`isDisplayOnly = true`), question components should render only the choice rows that were selected — by the learner, the reviewer, or both. unselected choices must not appear. +CORE-8225 selected-only rendering remains the rule for read-only checkbox feedback. Show the union +of choices selected by the learner or reviewer; never show unselected choices. If both selected the +same choice, render it once with both ownership labels. -## components affected +Reviewer-only groups are hidden from learner answering and pending-review views. After feedback is +published, every group containing only reviewer-only questions is shown under **Reviewer Feedback**. +The section establishes answer ownership, so its responses do not repeat **Reviewer's Answer**. -| component | question type | answer shape | +## display behaviour + +| context | choices | ownership labels | |---|---|---| -| `app-multiple` | multiple choice (checkboxes) | array of ids | -| `app-oneof` | single choice (radio) | scalar id | +| learner viewing a shared question | selected learner/reviewer values only | **Your Answer** and **Reviewer's Answer** | +| reviewer viewing a shared question | selected learner/reviewer values only | **Learner's Answer** and **Reviewer's Answer** | +| learner viewing **Reviewer Feedback** | reviewer-selected values only | none; the section supplies context | +| learner answering or waiting for review | reviewer-only groups hidden | existing authoring behaviour | +| reviewer authoring or viewing a review | configured groups and order unchanged | existing reviewer behaviour | ## implementation -### app-multiple (`multiple.component.ts` / `multiple.component.html`) -- `displayChoices` getter collects all selected ids from `submission.answer` (array) and `review.answer` (array) into a `Set`, then filters `question.choices` to matching entries. -- handles stringified and nested answer payloads via `_collectSelectedChoiceIds()`. -- template iterates `displayChoices` instead of `question.choices` inside the `*ngIf="isDisplayOnly"` branch. +### assessment groups + +- A group is reviewer-only when it has at least one question and every question is marked + `reviewerOnly`, or has `reviewer` as its sole audience. +- `displayGroups` removes reviewer-only groups before publication and appends them after ordinary + learner groups once feedback is available. +- `isReviewerFeedbackContext` is passed explicitly to question components. Do not infer this only + from `question.reviewerOnly`. It suppresses redundant ownership labels for published learner + feedback and completed reviewer views, while shared groups and pending review authoring retain + their labels. +- Pending reviewers see one neutral **Reviewer-only questions** guidance callout at the start of + each contiguous reviewer-only section, including when pagination begins within that section. +- Empty reviewer arrays, strings, and objects use the neutral **No answer for this question** state. + +### choice questions + +- `app-multiple` collects learner and reviewer answer ids, including stringified or nested payloads, + and filters configured choices to the selected union. +- `app-oneof` remains selected-only and renders both selected rows when learner and reviewer chose + different values. +- In shared-question read-only views, ownership chips in choice and team-member selector questions + render before the selected answer content, matching the text and file question layout. +- Reviewer-only team selectors filter their read-only list to reviewer-selected members. +- Selection inputs remain unchanged during learner and reviewer authoring. + +### other reviewer-only answers -### app-oneof (`oneof.component.ts` / `oneof.component.html`) -- `displayChoices` getter adds `submission.answer` and `review.answer` (both scalars) into a `Set`, then filters `question.choices`. -- if the learner and reviewer selected different options, both rows are shown — each with its own "Learner's Answer" / "Reviewer's Answer" chip. -- if they selected the same option, the Set deduplicates it; the single row shows both chips. -- template iterates `displayChoices` instead of `question.choices` inside the `*ngIf="isDisplayOnly"` branch. +- Text and file/video responses render their content without a reviewer ownership chip. +- Sliders use the review answer as the disabled scale value without a reviewer ownership chip. +- Genuine content labels such as **Feedback** remain visible. ## verification -- `projects/v3/src/app/components/multiple/multiple.component.spec.ts` covers the filtered preview list and the rendered template output for the multiple type. \ No newline at end of file +- Assessment tests cover reviewer-only visibility, ordering, pagination, authoring guidance, + context, and empty answers. +- Choice component tests cover selected unions, shared selections, reviewer-only selections, and + learner/reviewer ownership labels. +- Text, slider, file/video, and selector checks cover contextual ownership suppression in published + learner feedback and completed reviewer views. diff --git a/projects/v3/src/app/components/assessment/assessment.component.html b/projects/v3/src/app/components/assessment/assessment.component.html index 4325d5f425..0dc3334f81 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.html +++ b/projects/v3/src/app/components/assessment/assessment.component.html @@ -173,6 +173,26 @@ i18n-aria-label novalidate> +
+

Reviewer Feedback

+

These criteria were completed by your reviewer.

+
+ +
+

Reviewer-only questions

+

Complete these questions as part of your review. Your answers will be shared with the learner after you submit the review.

+
+
@@ -271,23 +291,18 @@ i18n-aria-label>
- - - - No answer for this question. - - + + + No answer for this question. + @@ -315,6 +332,8 @@ [review]="review?.answers[question.id] || {}" [reviewStatus]="review ? review.status : ''" [submissionStatus]="submission ? submission.status : ''" + [viewerRole]="action === 'assessment' ? 'learner' : 'reviewer'" + [isReviewerFeedbackContext]="isReviewerOnlyReadOnlyGroup(group)" [formControlName]="'q-' + question.id" [control]="questionsForm?.controls['q-' + question.id]" [submitActions$]="submitActions"> @@ -330,6 +349,8 @@ [review]="review?.answers[question.id] || {}" [reviewStatus]="review ? review.status : ''" [submissionStatus]="submission ? submission.status : ''" + [viewerRole]="action === 'assessment' ? 'learner' : 'reviewer'" + [isReviewerFeedbackContext]="isReviewerOnlyReadOnlyGroup(group)" [formControlName]="'q-' + question.id" [control]="questionsForm?.controls['q-' + question.id]" [submitActions$]="submitActions"> @@ -345,6 +366,8 @@ [review]="review?.answers[question.id] || {}" [reviewStatus]="review ? review.status : ''" [submissionStatus]="submission ? submission.status : ''" + [viewerRole]="action === 'assessment' ? 'learner' : 'reviewer'" + [isReviewerFeedbackContext]="isReviewerOnlyReadOnlyGroup(group)" [formControlName]="'q-' + question.id" [control]="questionsForm?.controls['q-' + question.id]" [submitActions$]="submitActions"> @@ -361,6 +384,8 @@ [review]="review?.answers[question.id] || {}" [reviewStatus]="review ? review.status : ''" [submissionStatus]="submission ? submission.status : ''" + [viewerRole]="action === 'assessment' ? 'learner' : 'reviewer'" + [isReviewerFeedbackContext]="isReviewerOnlyReadOnlyGroup(group)" [control]="questionsForm?.controls['q-' + question.id]" [videoOnly]="true" [submitActions$]="submitActions"> @@ -377,6 +402,8 @@ [review]="review?.answers[question.id] || {}" [reviewStatus]="review ? review.status : ''" [submissionStatus]="submission ? submission.status : ''" + [viewerRole]="action === 'assessment' ? 'learner' : 'reviewer'" + [isReviewerFeedbackContext]="isReviewerOnlyReadOnlyGroup(group)" [control]="questionsForm?.controls['q-' + question.id]" [submitActions$]="submitActions"> @@ -391,6 +418,8 @@ [review]="review?.answers[question.id] || {}" [reviewStatus]="review ? review.status : ''" [submissionStatus]="submission ? submission.status : ''" + [viewerRole]="action === 'assessment' ? 'learner' : 'reviewer'" + [isReviewerFeedbackContext]="isReviewerOnlyReadOnlyGroup(group)" [formControlName]="'q-' + question.id" [control]="questionsForm?.controls['q-' + question.id]" [submitActions$]="submitActions"> @@ -406,6 +435,8 @@ [review]="review?.answers[question.id] || {}" [reviewStatus]="review ? review.status : ''" [submissionStatus]="submission ? submission.status : ''" + [viewerRole]="action === 'assessment' ? 'learner' : 'reviewer'" + [isReviewerFeedbackContext]="isReviewerOnlyReadOnlyGroup(group)" [formControlName]="'q-' + question.id" [control]="questionsForm?.controls['q-' + question.id]" [submitActions$]="submitActions"> @@ -443,6 +474,7 @@ (handleResubmit)="resubmit()" [text]="btnText" [disabled$]="btnDisabled$" + [showLoadingOnClick]="showSubmitLoadingOnClick" (handleClick)="continueToNextTask()" [hasCustomContent]="isPaginationEnabled && pageCount > 1"> diff --git a/projects/v3/src/app/components/assessment/assessment.component.scss b/projects/v3/src/app/components/assessment/assessment.component.scss index b0eace2ead..46d64bbde4 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.scss +++ b/projects/v3/src/app/components/assessment/assessment.component.scss @@ -138,6 +138,30 @@ ion-footer { margin-bottom: 10px; } +.reviewer-feedback-section, +.reviewer-only-guidance { + margin: 24px 0 8px; + padding: 16px; + background: var(--practera-light-grey-50); + + h3, + p { + margin: 0; + } + + p { + margin-top: 4px; + } +} + +.reviewer-feedback-section { + border-left: 4px solid var(--ion-color-success); +} + +.reviewer-only-guidance { + border-left: 4px solid var(--practera-dark-blue-100); +} + // styles use for due dates and pulsing animation .due-date { margin-bottom: 0; diff --git a/projects/v3/src/app/components/assessment/assessment.component.spec.ts b/projects/v3/src/app/components/assessment/assessment.component.spec.ts index c6bf94fc06..c58344d2c1 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.spec.ts +++ b/projects/v3/src/app/components/assessment/assessment.component.spec.ts @@ -1227,6 +1227,20 @@ describe('AssessmentComponent', () => { }); describe('continueToNextTask()', () => { + it('should enable loading-on-click only for submit actions', () => { + component.doAssessment = true; + component.isPendingReview = false; + expect(component.showSubmitLoadingOnClick).toBeTrue(); + + component.doAssessment = false; + component.isPendingReview = true; + expect(component.showSubmitLoadingOnClick).toBeTrue(); + + component.isPendingReview = false; + component.submission = { ...mockSubmission, status: 'done' } as any; + expect(component.showSubmitLoadingOnClick).toBeFalse(); + }); + it('should submit assessment', async () => { component.doAssessment = true; expect(component.btnText).toEqual('submit answers'); @@ -1658,6 +1672,44 @@ describe('AssessmentComponent', () => { }); describe('ngOnChanges() submitting flag preservation', () => { + it('should keep the review button disabled when an in-progress review is refetched during submit', () => { + component.action = 'review'; + component.assessment = { ...mockAssessment, type: 'moderated' } as any; + component.submission = { ...mockSubmission, status: 'pending review' } as any; + component.review = { ...mockReview, status: 'in progress' } as any; + component['submitting'] = true; + component.btnDisabled$.next(true); + + component.ngOnChanges({ + submission: { + previousValue: component.submission, + currentValue: component.submission, + firstChange: false, + isFirstChange: () => false, + }, + review: { + previousValue: component.review, + currentValue: component.review, + firstChange: false, + isFirstChange: () => false, + }, + } as any); + + expect(component['submitting']).toBeTrue(); + expect(component.btnDisabled$.getValue()).toBeTrue(); + }); + + it('should enable the review button when an in-progress review loads outside submission', () => { + component.isPendingReview = true; + component.review = { ...mockReview, status: 'in progress' } as any; + component['submitting'] = false; + component.btnDisabled$.next(true); + + component['_handleReviewData'](); + + expect(component.btnDisabled$.getValue()).toBeFalse(); + }); + it('should preserve submitting=true when same submission is refetched during submit', () => { // simulate initial state: user clicked submit component.ngOnChanges({ @@ -2377,6 +2429,222 @@ describe('AssessmentComponent', () => { }); }); + describe('CORE-8277: reviewer-only feedback group visibility', () => { + const learnerGroup = { + name: 'Learner Submission', + description: '', + questions: [{ id: 1, audience: ['submitter'], reviewerOnly: false }], + } as any; + const reviewerGroup = { + name: 'Client Criteria', + description: '', + questions: [{ id: 2, audience: ['reviewer'], reviewerOnly: true }], + } as any; + const secondReviewerGroup = { + name: 'Internal Notes', + description: '', + questions: [{ id: 3, audience: ['reviewer'], reviewerOnly: true }], + } as any; + + beforeEach(() => { + component.action = 'assessment'; + component.assessment = { + groups: [reviewerGroup, learnerGroup, secondReviewerGroup], + } as any; + component.submission = { status: 'feedback available' } as any; + }); + + it('should append every reviewer-only group after learner groups', () => { + expect(component.displayGroups.map(group => group.name)).toEqual([ + 'Learner Submission', + 'Client Criteria', + 'Internal Notes', + ]); + }); + + it('should require every question in a non-empty group to be reviewer-only', () => { + const mixedGroup = { + name: 'Mixed Questions', + questions: [ + { id: 4, audience: ['reviewer'], reviewerOnly: true }, + { id: 5, audience: ['submitter', 'reviewer'], reviewerOnly: false }, + ], + } as any; + const emptyGroup = { name: 'Empty', questions: [] } as any; + + expect(component.isReviewerOnlyGroup(reviewerGroup)).toBeTrue(); + expect(component.isReviewerOnlyGroup(mixedGroup)).toBeFalse(); + expect(component.isReviewerOnlyGroup(emptyGroup)).toBeFalse(); + }); + + it('should identify the first group in the dedicated reviewer feedback section', () => { + const groups = component.displayGroups; + + expect(component.isFirstReviewerFeedbackGroup(0, groups)).toBeFalse(); + expect(component.isFirstReviewerFeedbackGroup(1, groups)).toBeTrue(); + }); + + it('should render one dedicated reviewer feedback heading', () => { + fixture.detectChanges(); + + const sections = fixture.nativeElement.querySelectorAll('.reviewer-feedback-section'); + expect(sections.length).toBe(1); + expect(sections[0].textContent).toContain('Reviewer Feedback'); + expect(sections[0].textContent).toContain('These criteria were completed by your reviewer.'); + }); + + it('should hide every reviewer-only group before feedback is published', () => { + component.submission = { status: 'pending review' } as any; + + expect(component.displayGroups.map(group => group.name)).toEqual(['Learner Submission']); + }); + + it('should show a neutral no-answer state for an empty reviewer checkbox answer', () => { + const question = { + id: 2, + type: 'multiple', + audience: ['reviewer'], + reviewerOnly: true, + } as any; + component.doAssessment = false; + component.isPendingReview = false; + component.review = { answers: { 2: { answer: [] } } } as any; + + expect(component.shouldShowNoAnswer(question)).toBeTrue(); + + component.review.answers[2].answer = [1]; + expect(component.shouldShowNoAnswer(question)).toBeFalse(); + }); + + it('should leave slider empty states to the slider component', () => { + const question = { + id: 2, + type: 'slider', + audience: ['reviewer'], + reviewerOnly: true, + } as any; + component.doAssessment = false; + component.review = { answers: { 2: { answer: null } } } as any; + + expect(component.shouldShowNoAnswer(question)).toBeFalse(); + }); + + it('should preserve configured groups and order for reviewer views', () => { + component.action = 'review'; + + expect(component.displayGroups.map(group => group.name)).toEqual([ + 'Client Criteria', + 'Learner Submission', + 'Internal Notes', + ]); + }); + + it('should suppress ownership labels in completed reviewer-only groups for reviewer views', () => { + component.action = 'review'; + component.doAssessment = false; + component.isPendingReview = false; + component.review = { ...mockReview, status: 'done' } as any; + + expect(component.isReviewerOnlyReadOnlyGroup(reviewerGroup)).toBeTrue(); + expect(component.isReviewerOnlyReadOnlyGroup(learnerGroup)).toBeFalse(); + }); + + it('should retain reviewer authoring labels while a review is pending', () => { + component.action = 'review'; + component.doAssessment = false; + component.isPendingReview = true; + component.review = { ...mockReview, status: 'in progress' } as any; + + expect(component.isReviewerOnlyReadOnlyGroup(reviewerGroup)).toBeFalse(); + }); + + it('should keep the reviewer feedback heading learner-specific', () => { + component.action = 'review'; + component.doAssessment = false; + component.isPendingReview = false; + component.review = { ...mockReview, status: 'done' } as any; + + expect(component.isReviewerOnlyReadOnlyGroup(reviewerGroup)).toBeTrue(); + expect(component.isReviewerFeedbackGroup(reviewerGroup)).toBeFalse(); + expect(component.isFirstReviewerFeedbackGroup(0, [reviewerGroup])).toBeFalse(); + }); + + it('should identify the first pending reviewer-only authoring group in a contiguous section', () => { + component.action = 'review'; + component.isPendingReview = true; + const groups = [learnerGroup, reviewerGroup, secondReviewerGroup]; + + expect(component.isFirstReviewerOnlyAuthoringGroup(0, groups)).toBeFalse(); + expect(component.isFirstReviewerOnlyAuthoringGroup(1, groups)).toBeTrue(); + expect(component.isFirstReviewerOnlyAuthoringGroup(2, groups)).toBeFalse(); + }); + + it('should identify each separate pending reviewer-only authoring section', () => { + component.action = 'review'; + component.isPendingReview = true; + const groups = [reviewerGroup, learnerGroup, secondReviewerGroup]; + + expect(component.isFirstReviewerOnlyAuthoringGroup(0, groups)).toBeTrue(); + expect(component.isFirstReviewerOnlyAuthoringGroup(1, groups)).toBeFalse(); + expect(component.isFirstReviewerOnlyAuthoringGroup(2, groups)).toBeTrue(); + }); + + it('should show pending reviewer-only guidance when a paginated page starts within the section', () => { + component.action = 'review'; + component.isPendingReview = true; + + expect(component.isFirstReviewerOnlyAuthoringGroup(0, [secondReviewerGroup])).toBeTrue(); + }); + + it('should hide reviewer-only authoring guidance outside pending reviewer views', () => { + component.action = 'review'; + component.isPendingReview = false; + expect(component.isFirstReviewerOnlyAuthoringGroup(0, [reviewerGroup])).toBeFalse(); + + component.action = 'assessment'; + component.isPendingReview = true; + expect(component.isFirstReviewerOnlyAuthoringGroup(0, [reviewerGroup])).toBeFalse(); + }); + + it('should render one accessible guidance callout for consecutive reviewer-only groups', () => { + component.action = 'review'; + component.assessment = { + groups: [learnerGroup, reviewerGroup, secondReviewerGroup], + } as any; + component.submission = { + ...mockSubmission, + status: 'pending review', + answers: {}, + } as any; + component.review = { ...mockReview, status: 'in progress' } as any; + component.doAssessment = false; + component.isPendingReview = true; + + fixture.detectChanges(); + + const guidance = fixture.nativeElement.querySelectorAll('.reviewer-only-guidance'); + expect(guidance.length).toBe(1); + expect(guidance[0].textContent).toContain('Reviewer-only questions'); + expect(guidance[0].textContent).toContain('Your answers will be shared with the learner'); + const headingId = guidance[0].querySelector('h3').id; + expect(headingId).toBe('reviewer-only-guidance-heading-0-1'); + expect(guidance[0].getAttribute('aria-labelledby')).toBe(headingId); + }); + + it('should use the filtered and reordered groups when building pagination', () => { + component.pageSize = 10; + + const pages = component['splitGroupsByQuestionCount'](); + + expect(pages.length).toBe(1); + expect(pages[0].map(group => group.name)).toEqual([ + 'Learner Submission', + 'Client Criteria', + 'Internal Notes', + ]); + }); + }); + describe('CORE-8182: pagination indicator accuracy in review mode', () => { const reviewAssessment: Assessment = { id: 1, diff --git a/projects/v3/src/app/components/assessment/assessment.component.ts b/projects/v3/src/app/components/assessment/assessment.component.ts index d808bd1a63..6809a74736 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.ts +++ b/projects/v3/src/app/components/assessment/assessment.component.ts @@ -34,6 +34,8 @@ interface Team360Section { kind: Team360SectionKind; } +type DisplayGroup = Pick & Partial; + /** * Assessment Component with optional pagination feature * @@ -178,7 +180,7 @@ export class AssessmentComponent implements OnInit, OnChanges, OnDestroy { pageIndex = 0; // each entry is a page: an array of (partial) groups - pagesGroups: { name: string; description?: string; questions: Question[] }[][] = []; + pagesGroups: DisplayGroup[][] = []; // Feature toggle for pagination get isPaginationEnabled(): boolean { @@ -238,11 +240,96 @@ export class AssessmentComponent implements OnInit, OnChanges, OnDestroy { get pagedGroups() { if (!this.isPaginationEnabled) { // Return all groups as a single page when pagination is disabled - return this.assessment?.groups || []; + return this.displayGroups; } return this.pagesGroups[this.pageIndex] || []; } + /** + * Groups visible to the current viewer. + * + * Reviewer-only groups are never part of the learner's answering or pending-review views. Once + * feedback is published, reviewer-only groups are appended after the learner groups so they can + * be presented as a dedicated feedback section. Reviewer views retain configured order. + */ + get displayGroups(): DisplayGroup[] { + const groups = this.assessment?.groups ?? []; + if (this.action !== 'assessment') { + return groups; + } + + const learnerGroups = groups.filter(group => !this.isReviewerOnlyGroup(group)); + if (this.submission?.status !== 'feedback available') { + return learnerGroups; + } + + const reviewerFeedbackGroups = groups.filter(group => this.isReviewerOnlyGroup(group)); + + return [...learnerGroups, ...reviewerFeedbackGroups]; + } + + isReviewerOnlyGroup(group: DisplayGroup): boolean { + return group?.questions?.length > 0 && group.questions.every(question => + question.reviewerOnly === true || + (question.audience?.length === 1 && question.audience.includes('reviewer')) + ); + } + + isReviewerFeedbackGroup(group: DisplayGroup): boolean { + return this.action === 'assessment' + && this.submission?.status === 'feedback available' + && this.isReviewerOnlyGroup(group); + } + + /** + * Whether answers in this group are already identified as reviewer-authored by their context. + * + * Learners reach this state after feedback is published. Reviewers reach it when reopening a + * completed review. Pending reviews remain outside this context so authoring controls are not + * affected, and shared groups retain their answer-ownership labels. + */ + isReviewerOnlyReadOnlyGroup(group: DisplayGroup): boolean { + if (!this.isReviewerOnlyGroup(group) || this.doAssessment || this.isPendingReview) { + return false; + } + + if (this.action === 'assessment') { + return this.submission?.status === 'feedback available'; + } + + return this.action === 'review' && this.review?.status === 'done'; + } + + isFirstReviewerFeedbackGroup(groupIndex: number, groups: DisplayGroup[]): boolean { + return this.isReviewerFeedbackGroup(groups[groupIndex]) + && (groupIndex === 0 || !this.isReviewerFeedbackGroup(groups[groupIndex - 1])); + } + + /** + * Show reviewer-only authoring guidance once per contiguous reviewer-only section. + * + * A reviewer-only group at the start of a paginated page is treated as the start of a section so + * the guidance remains available when the preceding group is rendered on another page. + */ + isFirstReviewerOnlyAuthoringGroup(groupIndex: number, groups: DisplayGroup[]): boolean { + return this.action === 'review' + && this.isPendingReview + && this.isReviewerOnlyGroup(groups[groupIndex]) + && (groupIndex === 0 || !this.isReviewerOnlyGroup(groups[groupIndex - 1])); + } + + shouldShowNoAnswer(question: Question): boolean { + if (this.doAssessment || question?.type === 'slider') { + return false; + } + + if (question?.reviewerOnly) { + return !this.isPendingReview && this.utils.isEmpty(this.review?.answers?.[question.id]?.answer); + } + + return this.utils.isEmpty(this.submission?.answers?.[question.id]?.answer); + } + prevPage() { if (!this.isPaginationEnabled) return; const accessiblePages = this.accessiblePageIndexes; @@ -708,7 +795,12 @@ Best regards`; private _handleReviewData() { if (this.isPendingReview && this.review?.status === 'in progress') { this.savingMessage$.next($localize`Last saved ${this.utils.timeFormatter(this.review.modified)}`); - this.btnDisabled$.next(false); + // An intermediate status-check fetch republishes the same in-progress review while the + // submit request is still running. Keep the action disabled until the parent submission + // workflow explicitly reports completion or failure. + if (!this.submitting) { + this.btnDisabled$.next(false); + } } } @@ -989,6 +1081,10 @@ Best regards`; return 'continue'; } + get showSubmitLoadingOnClick(): boolean { + return this._btnAction === 'submit'; + } + // the text of the button get btnText() { switch (this._btnAction) { @@ -1148,14 +1244,14 @@ Best regards`; */ private splitGroupsByQuestionCount() { if (this.isTeam360Assessment) { - return (this.assessment?.groups ?? []).map(group => [group]); + return this.displayGroups.map(group => [group]); } const pages = []; let currentPage = []; let count = 0; - for (const group of this.assessment.groups) { + for (const group of this.displayGroups) { const qCount = group.questions.length; if (count + qCount <= this.pageSize) { diff --git a/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.html b/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.html index d4e86a950b..3795bbd3b3 100644 --- a/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.html +++ b/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.html @@ -11,15 +11,25 @@
{{ text }} + > + + {{ text }} + { beforeEach(async () => { await TestBed.configureTestingModule({ declarations: [BottomActionBarComponent], + schemas: [CUSTOM_ELEMENTS_SCHEMA], }).compileComponents(); }); @@ -31,6 +33,8 @@ describe('BottomActionBarComponent', () => { expect(component.buttonType).toBe(''); expect(component.hasCustomContent).toBe(false); expect(component.disabled$).toBeUndefined(); + expect(component.showLoadingOnClick).toBe(false); + expect(component.loading).toBe(false); }); }); @@ -88,6 +92,63 @@ describe('BottomActionBarComponent', () => { expect(component.handleClick.emit).toHaveBeenCalledWith(clickEvent); }); + + it('should show loading immediately and prevent duplicate clicks when opted in', () => { + const disabled$ = new BehaviorSubject(false); + fixture.componentRef.setInput('disabled$', disabled$); + fixture.componentRef.setInput('showLoadingOnClick', true); + fixture.detectChanges(); + spyOn(component.handleClick, 'emit'); + + const clickEvent = new MouseEvent('click'); + component.onClick(clickEvent); + component.onClick(clickEvent); + fixture.detectChanges(); + + expect(component.loading).toBeTrue(); + expect(component.handleClick.emit).toHaveBeenCalledTimes(1); + expect(fixture.debugElement.query(By.css('ion-spinner.action-spinner'))).toBeTruthy(); + expect(fixture.debugElement.query(By.css('.button-container.is-loading'))).toBeTruthy(); + + const actionButton = fixture.debugElement.query(By.css('ion-button.action-button')); + expect(actionButton.properties.disabled).toBeTrue(); + expect(actionButton.attributes['aria-busy']).toBe('true'); + }); + + it('should clear loading when disabled$ emits false after processing', () => { + const disabled$ = new BehaviorSubject(false); + fixture.componentRef.setInput('disabled$', disabled$); + fixture.componentRef.setInput('showLoadingOnClick', true); + fixture.detectChanges(); + + component.onClick(new MouseEvent('click')); + disabled$.next(true); + expect(component.loading).toBeTrue(); + + disabled$.next(false); + fixture.detectChanges(); + + expect(component.loading).toBeFalse(); + expect(fixture.debugElement.query(By.css('ion-spinner.action-spinner'))).toBeNull(); + }); + + it('should not enter loading or emit when already disabled', () => { + fixture.componentRef.setInput('disabled$', new BehaviorSubject(true)); + fixture.componentRef.setInput('showLoadingOnClick', true); + fixture.detectChanges(); + spyOn(component.handleClick, 'emit'); + + component.onClick(new MouseEvent('click')); + fixture.detectChanges(); + + expect(component.loading).toBeFalse(); + expect(component.handleClick.emit).not.toHaveBeenCalled(); + expect(fixture.debugElement.query(By.css('ion-spinner.action-spinner'))).toBeNull(); + + const actionButton = fixture.debugElement.query(By.css('ion-button.action-button')); + expect(actionButton.properties.disabled).toBeTrue(); + expect(actionButton.attributes['aria-busy']).toBe('false'); + }); }); describe('onResubmit()', () => { diff --git a/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.ts b/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.ts index e1b8df616c..37c9750c6b 100644 --- a/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.ts +++ b/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.ts @@ -1,5 +1,5 @@ -import { Component, Input, Output, EventEmitter, OnChanges } from '@angular/core'; -import { BehaviorSubject } from 'rxjs'; +import { Component, Input, Output, EventEmitter, OnChanges, OnDestroy, SimpleChanges } from '@angular/core'; +import { BehaviorSubject, Subscription } from 'rxjs'; @Component({ standalone: false, @@ -7,26 +7,51 @@ import { BehaviorSubject } from 'rxjs'; templateUrl: 'bottom-action-bar.component.html', styleUrls: ['./bottom-action-bar.component.scss'], }) -export class BottomActionBarComponent { +export class BottomActionBarComponent implements OnChanges, OnDestroy { @Input() showResubmit: boolean = false; @Input() text: string; @Input() color: string = 'primary'; @Input() disabled$?: BehaviorSubject; // assessment only + @Input() showLoadingOnClick: boolean = false; @Output() handleClick = new EventEmitter(); @Output() handleResubmit = new EventEmitter(); @Input() buttonType: string = ''; @Input() hasCustomContent: boolean = false; + loading = false; + + private disabledSubscription?: Subscription; + constructor() {} + ngOnChanges(changes: SimpleChanges): void { + if (!changes.disabled$) { + return; + } + + this.disabledSubscription?.unsubscribe(); + this.disabledSubscription = this.disabled$?.subscribe(disabled => { + if (disabled === false) { + this.loading = false; + } + }); + } + + ngOnDestroy(): void { + this.disabledSubscription?.unsubscribe(); + } + onClick(clickEvent: Event) { - // if disabled, do nothing - if (this.disabled$?.getValue() === true) { + // if disabled or already processing, do nothing + if (this.disabled$?.getValue() === true || this.loading) { return; } // make sure it's the click event that triggers "handleClick" if (clickEvent.type === 'click') { + if (this.showLoadingOnClick) { + this.loading = true; + } return this.handleClick.emit(clickEvent); } @@ -37,4 +62,3 @@ export class BottomActionBarComponent { return this.handleResubmit.emit(clickEvent); } } - diff --git a/projects/v3/src/app/components/file-upload/file-upload.component.html b/projects/v3/src/app/components/file-upload/file-upload.component.html index 6ca3a6fc39..f5edb34058 100644 --- a/projects/v3/src/app/components/file-upload/file-upload.component.html +++ b/projects/v3/src/app/components/file-upload/file-upload.component.html @@ -1,9 +1,12 @@
- + -

Learner's Answer

+

+ Your Answer + Learner's Answer +

@@ -16,9 +19,9 @@ - + -

Reviewer's answer

+

Reviewer's Answer

No answer given to this optional question.

diff --git a/projects/v3/src/app/components/file-upload/file-upload.component.ts b/projects/v3/src/app/components/file-upload/file-upload.component.ts index 64873b627a..0298e4ec06 100644 --- a/projects/v3/src/app/components/file-upload/file-upload.component.ts +++ b/projects/v3/src/app/components/file-upload/file-upload.component.ts @@ -66,6 +66,8 @@ export class FileUploadComponent implements OnInit, OnDestroy { // assessment/review action flags @Input() doAssessment: boolean; @Input() doReview: boolean; + @Input() viewerRole: 'learner' | 'reviewer'; + @Input() isReviewerFeedbackContext = false; // FormControl that is passed in from parent component @Input() control: AbstractControl; diff --git a/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.html b/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.html index 5bdd5c5967..e8ae8db4a0 100644 --- a/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.html +++ b/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.html @@ -1,21 +1,24 @@

{{question.name}}

- + - - +

- Learner's Answer + + Your Answer + Learner's Answer +

- +

Reviewer's Answer

+ diff --git a/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.spec.ts b/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.spec.ts index 5a1ee65d50..d213663899 100644 --- a/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.spec.ts +++ b/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.spec.ts @@ -41,6 +41,52 @@ describe('MultiTeamMemberSelectorComponent', () => { expect(component).toBeDefined(); }); + it('should show only reviewer-selected members in reviewer feedback', () => { + const member1 = JSON.stringify({ userId: 1 }); + const member2 = JSON.stringify({ userId: 2 }); + component.question = { + audience: ['reviewer'], + teamMembers: [ + { key: member1, userName: 'Member 1' }, + { key: member2, userName: 'Member 2' }, + ], + } as any; + component.review = { answer: [member2] }; + component.isReviewerFeedbackContext = true; + + expect(component.displayTeamMembers.map(member => member.key)).toEqual([member2]); + + component.isReviewerFeedbackContext = false; + expect(component.displayTeamMembers.length).toBe(2); + }); + + it('should render the learner ownership chip before the selected member', () => { + const member1 = JSON.stringify({ userId: 1 }); + const member2 = JSON.stringify({ userId: 2 }); + component.question = { + audience: ['submitter', 'reviewer'], + teamMembers: [ + { key: member1, userName: 'Member 1' }, + { key: member2, userName: 'Member 2' }, + ], + } as any; + component.submissionStatus = 'feedback available'; + component.reviewStatus = 'done'; + component.doAssessment = false; + component.doReview = false; + component.viewerRole = 'learner'; + component.submission = { answer: [member1] }; + component.review = { answer: [member2] }; + + fixture.detectChanges(); + + const learnerItem = fixture.nativeElement.querySelector('ion-list ion-item'); + const labelChildren = Array.from(learnerItem.querySelector('ion-label').children); + expect(labelChildren.indexOf(learnerItem.querySelector('p'))) + .toBeLessThan(labelChildren.indexOf(learnerItem.querySelector('.answer-content'))); + expect(learnerItem.textContent).toContain('Your Answer'); + }); + describe('ngOnInit()', () => { it('should call _showSavedAnswers()', () => { // use "any" to bypass ts restriction on type (not recommended, for acceptable for internal implementation) diff --git a/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.ts b/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.ts index 5597b283dd..6fae9877a8 100644 --- a/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.ts +++ b/projects/v3/src/app/components/multi-team-member-selector/multi-team-member-selector.component.ts @@ -33,6 +33,8 @@ export class MultiTeamMemberSelectorComponent implements ControlValueAccessor, O @Input() doAssessment: Boolean; // this is for doing review or not @Input() doReview: Boolean; + @Input() viewerRole: 'learner' | 'reviewer'; + @Input() isReviewerFeedbackContext = false; // FormControl that is passed in from parent component @Input() control: AbstractControl<{answer: string[], comment: string}>; // answer field for submitter & reviewer @@ -208,6 +210,15 @@ export class MultiTeamMemberSelectorComponent implements ControlValueAccessor, O return !this.doAssessment && !this.doReview && (this.submissionStatus === 'feedback available' || this.submissionStatus === 'pending review' || (this.submissionStatus === 'done' && this.reviewStatus === '')) && (this.submission?.answer || this.review?.answer); } + get displayTeamMembers(): Array { + const teamMembers = this.question?.teamMembers || []; + if (!this.isReviewerFeedbackContext) { + return teamMembers; + } + + return teamMembers.filter(teamMember => this.isSelectedInReview(teamMember)); + } + /** * checks if a team member is selected using the local working state (innerValue). * reads from innerValue.answer in review mode, or innerValue directly in assessment mode. diff --git a/projects/v3/src/app/components/multiple/multiple.component.html b/projects/v3/src/app/components/multiple/multiple.component.html index 0a72dc4579..f0e5aa69b6 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.html +++ b/projects/v3/src/app/components/multiple/multiple.component.html @@ -3,20 +3,25 @@

{ + [ngClass]="{'item-bottom-border': !(choice.explanation && choice.explanation.changingThisBreaksApplicationSecurity && isSubmissionChoiceSelected(choice.id))}"> -
- Learner's Answer - Reviewer's Answer + + + Your Answer + Learner's Answer + + + Reviewer's Answer + + +
- + diff --git a/projects/v3/src/app/components/multiple/multiple.component.scss b/projects/v3/src/app/components/multiple/multiple.component.scss index 46fc455458..914aca52ec 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.scss +++ b/projects/v3/src/app/components/multiple/multiple.component.scss @@ -64,4 +64,3 @@ ion-item { .feedback-title { --min-height: 1em; } - diff --git a/projects/v3/src/app/components/multiple/multiple.component.spec.ts b/projects/v3/src/app/components/multiple/multiple.component.spec.ts index a0685aa4eb..d9b8112973 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.spec.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.spec.ts @@ -136,47 +136,104 @@ describe('MultipleComponent', () => { }); describe('when testing display-only preview mode', () => { - it('should expose only selected choices', () => { + beforeEach(() => { component.question = { choices: [ { id: 1, name: 'choice1' }, { id: 2, name: 'choice2' }, - { id: 3, name: 'choice3' } + { id: 3, name: 'choice3' }, + { id: 4, name: 'choice4' } ], - audience: [] + audience: ['submitter', 'reviewer'] }; component.submissionStatus = 'feedback available'; + component.reviewStatus = 'done'; component.doAssessment = false; component.doReview = false; - component.submission = { answer: [2] }; + component.viewerRole = 'reviewer'; + component.submission = { answer: [1, 4] }; + component.review = { answer: [2, 4] }; + }); + it('should render only the union of learner and reviewer selections', () => { fixture.detectChanges(); - expect(component.isDisplayOnly).toBeTrue(); - expect(component.displayChoices.map(choice => choice.id)).toEqual([2]); + const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); + expect(items.length).toBe(3); + expect(items[0].textContent).toContain('choice1'); + expect(items[0].textContent).toContain("Learner's Answer"); + expect(items[1].textContent).toContain('choice2'); + expect(items[1].textContent).toContain("Reviewer's Answer"); + expect(items[2].textContent).toContain('choice4'); + expect(items[2].textContent).toContain("Learner's Answer"); + expect(items[2].textContent).toContain("Reviewer's Answer"); + expect(fixture.nativeElement.textContent).not.toContain('choice3'); + expect(fixture.nativeElement.textContent).not.toContain('Not Selected'); }); - it('should render only selected choices in the template', () => { - component.question = { - choices: [ - { id: 1, name: 'choice1' }, - { id: 2, name: 'choice2' }, - { id: 3, name: 'choice3' } - ], - audience: [] - }; - component.submissionStatus = 'feedback available'; - component.doAssessment = false; - component.doReview = false; - component.submission = { answer: [2] }; + it('should use Your Answer for learner selections in learner view', () => { + component.viewerRole = 'learner'; + + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toContain('Your Answer'); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + + const learnerItem = fixture.nativeElement.querySelector('ion-list ion-item'); + const labelChildren = Array.from(learnerItem.querySelector('ion-label').children); + expect(labelChildren.indexOf(learnerItem.querySelector('ion-chip'))) + .toBeLessThan(labelChildren.indexOf(learnerItem.querySelector('.answer-content'))); + }); + + it('should render a shared choice once with both ownership labels', () => { + component.submission = { answer: [4] }; + component.review = { answer: [4] }; fixture.detectChanges(); const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); expect(items.length).toBe(1); - expect(fixture.nativeElement.textContent).toContain('choice2'); + expect(items[0].textContent).toContain("Learner's Answer"); + expect(items[0].textContent).toContain("Reviewer's Answer"); + }); + + it('should show only reviewer selections without ownership labels in reviewer feedback', () => { + component.question.audience = ['reviewer']; + component.submission = {}; + component.isReviewerFeedbackContext = true; + component.viewerRole = 'learner'; + + fixture.detectChanges(); + + const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); + expect(items.length).toBe(2); + expect(items[0].textContent).toContain('choice2'); + expect(items[1].textContent).toContain('choice4'); expect(fixture.nativeElement.textContent).not.toContain('choice1'); expect(fixture.nativeElement.textContent).not.toContain('choice3'); + expect(fixture.nativeElement.querySelectorAll('ion-chip').length).toBe(0); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); + }); + + it('should render no choices for an empty reviewer-only answer', () => { + component.question.audience = ['reviewer']; + component.submission = {}; + component.review = { answer: [] }; + component.isReviewerFeedbackContext = true; + + fixture.detectChanges(); + + expect(component.displayChoices).toEqual([]); + expect(fixture.nativeElement.querySelectorAll('ion-list ion-item').length).toBe(0); + }); + + it('should normalise stringified selected choice arrays', () => { + component.submission = { answer: '[1]' }; + component.review = { answer: { answer: '[2]' } }; + + fixture.detectChanges(); + + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); }); }); diff --git a/projects/v3/src/app/components/multiple/multiple.component.ts b/projects/v3/src/app/components/multiple/multiple.component.ts index 8cd087be04..90f4584c9b 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.ts @@ -34,6 +34,9 @@ export class MultipleComponent implements AfterViewInit, ControlValueAccessor, O @Input() doAssessment: Boolean; // this is for doing review or not @Input() doReview: Boolean; + // role of the user viewing completed feedback + @Input() viewerRole: 'learner' | 'reviewer'; + @Input() isReviewerFeedbackContext = false; // FormControl that is passed in from parent component @Input() control: AbstractControl; // comment field for reviewer @@ -259,6 +262,20 @@ export class MultipleComponent implements AfterViewInit, ControlValueAccessor, O return (this.question?.choices || []).filter(choice => selectedChoiceIds.has(choice.id)); } + isReviewerChoiceSelected(choiceId: string | number): boolean { + return this._answerIncludesChoice(this.review?.answer, choiceId); + } + + isSubmissionChoiceSelected(choiceId: string | number): boolean { + return this._answerIncludesChoice(this.submission?.answer, choiceId); + } + + private _answerIncludesChoice(answer: any, choiceId: string | number): boolean { + const selectedChoiceIds = new Set(); + this._collectSelectedChoiceIds(answer, selectedChoiceIds); + return selectedChoiceIds.has(choiceId); + } + private _collectSelectedChoiceIds(answer: any, selectedChoiceIds: Set): void { if (answer === null || answer === undefined) { return; diff --git a/projects/v3/src/app/components/oneof/oneof.component.html b/projects/v3/src/app/components/oneof/oneof.component.html index 53ba85301a..22c4a27e1d 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.html +++ b/projects/v3/src/app/components/oneof/oneof.component.html @@ -6,13 +6,19 @@

{{question. -
- Learner's Answer - Reviewer's Answer + + + Your Answer + Learner's Answer + + Reviewer's Answer + +
diff --git a/projects/v3/src/app/components/oneof/oneof.component.spec.ts b/projects/v3/src/app/components/oneof/oneof.component.spec.ts index 4d63ed46eb..46af932b52 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.spec.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.spec.ts @@ -42,6 +42,54 @@ describe('OneofComponent', () => { expect(component).toBeDefined(); }); + describe('read-only ownership context', () => { + beforeEach(() => { + component.question = { + id: 1, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + ], + audience: ['submitter', 'reviewer'], + }; + component.submissionStatus = 'feedback available'; + component.reviewStatus = 'done'; + component.doAssessment = false; + component.doReview = false; + component.submission = { answer: 1 }; + component.review = { answer: 2 }; + }); + + it('should use ownership labels for a shared question', () => { + component.viewerRole = 'learner'; + + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toContain('Your Answer'); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + + const learnerItem = fixture.nativeElement.querySelector('ion-list ion-item'); + const labelChildren = Array.from(learnerItem.querySelector('ion-label').children); + expect(labelChildren.indexOf(learnerItem.querySelector('ion-chip'))) + .toBeLessThan(labelChildren.indexOf(learnerItem.querySelector('.answer-content'))); + }); + + it('should show only the selected value without a label in reviewer feedback', () => { + component.question.audience = ['reviewer']; + component.submission = {}; + component.isReviewerFeedbackContext = true; + + fixture.detectChanges(); + + const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); + expect(items.length).toBe(1); + expect(items[0].textContent).toContain('choice2'); + expect(fixture.nativeElement.querySelectorAll('ion-chip').length).toBe(0); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); + }); + }); + describe('when testing onInit()', () => { it('should get correct data for in progress submission', () => { component.question = { diff --git a/projects/v3/src/app/components/oneof/oneof.component.ts b/projects/v3/src/app/components/oneof/oneof.component.ts index 132d577f99..45e03069fe 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.ts @@ -32,6 +32,8 @@ export class OneofComponent implements AfterViewInit, ControlValueAccessor, OnIn @Input() doAssessment: Boolean; // this is for doing review or not @Input() doReview: Boolean; + @Input() viewerRole: 'learner' | 'reviewer'; + @Input() isReviewerFeedbackContext = false; // FormControl that is passed in from parent component @Input() control: AbstractControl; // answer field for submitter & reviewer diff --git a/projects/v3/src/app/components/slider/slider.component.html b/projects/v3/src/app/components/slider/slider.component.html index 87cf64444b..9c0438b06a 100644 --- a/projects/v3/src/app/components/slider/slider.component.html +++ b/projects/v3/src/app/components/slider/slider.component.html @@ -2,8 +2,8 @@

{{question
- -
+ +
{{question [ticks]="true" [pin]="true" [pinFormatter]="pinFormatter" - [value]="getSubmissionSliderValue()" + [value]="getDisplaySliderValue()" disabled [attr.aria-labelledby]="'slider-question-' + question.id" color="medium" @@ -21,20 +21,26 @@

{{question
-
+
- - + +
-

No answer provided

-

The learner has not submitted an answer for this question.

+ +

No reviewer answer provided

+

The reviewer has not submitted an answer for this question.

+ + +

No answer provided

+

The learner has not submitted an answer for this question.

+
@@ -43,20 +49,19 @@

{{question
-
- - - Learner's Answer: {{ getChoiceNameById(submission.answer) }} +
+ + Your Answer: {{ getChoiceNameById(submission.answer) }} + Learner's Answer: {{ getChoiceNameById(submission.answer) }} - - + Reviewer's Answer: {{ getChoiceNameById(review.answer) }}
-
+
No answers available yet diff --git a/projects/v3/src/app/components/slider/slider.component.scss b/projects/v3/src/app/components/slider/slider.component.scss index fdd5eb9d91..9c365b041f 100644 --- a/projects/v3/src/app/components/slider/slider.component.scss +++ b/projects/v3/src/app/components/slider/slider.component.scss @@ -226,7 +226,6 @@ ion-item { .label { align-self: flex-start; - font-size: 0.9rem; &.orange { --background: var(--ion-color-warning-tint); diff --git a/projects/v3/src/app/components/slider/slider.component.spec.ts b/projects/v3/src/app/components/slider/slider.component.spec.ts index b6b79a00f8..1ffa504658 100644 --- a/projects/v3/src/app/components/slider/slider.component.spec.ts +++ b/projects/v3/src/app/components/slider/slider.component.spec.ts @@ -341,6 +341,69 @@ describe('SliderComponent', () => { }); }); + describe('reviewer-only display', () => { + beforeEach(() => { + component.question = { + ...component.question, + audience: ['reviewer'], + reviewerOnly: true, + }; + component.doAssessment = false; + component.doReview = false; + component.submissionStatus = 'feedback available'; + component.submission = {}; + component.viewerRole = 'learner'; + component.isReviewerFeedbackContext = true; + }); + + it('should use the reviewer answer as the displayed slider value', () => { + component.review = { answer: 3 }; + + fixture.detectChanges(); + + expect(component.hasDisplaySliderAnswer()).toBeTrue(); + expect(component.getDisplaySliderValue()).toBe(3); + expect(fixture.nativeElement.querySelector('ion-range.display-only-slider')).toBeTruthy(); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); + expect(fixture.nativeElement.querySelectorAll('ion-chip').length).toBe(0); + expect(fixture.nativeElement.textContent).not.toContain('The learner has not submitted an answer'); + }); + + it('should use reviewer-specific wording when no review answer exists', () => { + component.review = { answer: null }; + + fixture.detectChanges(); + + expect(component.hasDisplaySliderAnswer()).toBeFalse(); + expect(fixture.nativeElement.textContent).toContain('No reviewer answer provided'); + expect(fixture.nativeElement.textContent).toContain('The reviewer has not submitted an answer'); + expect(fixture.nativeElement.textContent).not.toContain('The learner has not submitted an answer'); + expect(fixture.nativeElement.textContent).not.toContain('No answers available yet'); + }); + }); + + it('should use ownership labels for a shared slider in learner view', () => { + component.question = { + ...component.question, + audience: ['submitter', 'reviewer'], + reviewerOnly: false, + }; + component.doAssessment = false; + component.doReview = false; + component.submissionStatus = 'feedback available'; + component.reviewStatus = 'done'; + component.viewerRole = 'learner'; + component.submission = { answer: 2 }; + component.review = { answer: 4 }; + + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toContain('Your Answer: 2'); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer: 4"); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + expect(fixture.nativeElement.querySelectorAll('.answer-indicators ion-icon').length).toBe(0); + }); + describe('onLabelClick guard', () => { it('should not call onChange when control is disabled', () => { component.ngOnInit(); diff --git a/projects/v3/src/app/components/slider/slider.component.ts b/projects/v3/src/app/components/slider/slider.component.ts index 80298a2d28..cac59e277d 100644 --- a/projects/v3/src/app/components/slider/slider.component.ts +++ b/projects/v3/src/app/components/slider/slider.component.ts @@ -33,6 +33,8 @@ export class SliderComponent implements AfterViewInit, ControlValueAccessor, OnI @Input() doAssessment: Boolean; // this is for doing review or not @Input() doReview: Boolean; + @Input() viewerRole: 'learner' | 'reviewer'; + @Input() isReviewerFeedbackContext = false; // FormControl that is passed in from parent component @Input() control: AbstractControl; @@ -267,6 +269,16 @@ export class SliderComponent implements AfterViewInit, ControlValueAccessor, OnI return typeof this.submission.answer === 'number' ? this.submission.answer : this.sliderMin; } + hasDisplaySliderAnswer(): boolean { + return this.question?.reviewerOnly ? this.hasReviewAnswer() : this.hasSubmissionAnswer(); + } + + getDisplaySliderValue(): number { + const answer = this.question?.reviewerOnly ? this.review?.answer : this.submission?.answer; + + return typeof answer === 'number' ? answer : this.sliderMin; + } + // Get slider value for review (Reviewer's answer) getReviewSliderValue(): number { if (!this.innerValue?.answer) return this.sliderMin; diff --git a/projects/v3/src/app/components/team-member-selector/team-member-selector.component.html b/projects/v3/src/app/components/team-member-selector/team-member-selector.component.html index 993d120af9..14a73be65d 100644 --- a/projects/v3/src/app/components/team-member-selector/team-member-selector.component.html +++ b/projects/v3/src/app/components/team-member-selector/team-member-selector.component.html @@ -3,14 +3,17 @@

- + - -

- Learner's Answer +

+ + Your Answer + Learner's Answer + Reviewer's Answer

+ diff --git a/projects/v3/src/app/components/team-member-selector/team-member-selector.component.spec.ts b/projects/v3/src/app/components/team-member-selector/team-member-selector.component.spec.ts index aa8a2d3a4e..cdcafbc4ae 100644 --- a/projects/v3/src/app/components/team-member-selector/team-member-selector.component.spec.ts +++ b/projects/v3/src/app/components/team-member-selector/team-member-selector.component.spec.ts @@ -34,6 +34,47 @@ describe('TeamMemberSelectorComponent', () => { expect(component).toBeDefined(); }); + it('should show only the reviewer-selected member in reviewer feedback', () => { + component.question = { + teamMembers: [ + { key: 'member-1', userName: 'Member 1' }, + { key: 'member-2', userName: 'Member 2' }, + ], + }; + component.review = { answer: 'member-2' }; + component.isReviewerFeedbackContext = true; + + expect(component.displayTeamMembers.map(member => member.key)).toEqual(['member-2']); + + component.isReviewerFeedbackContext = false; + expect(component.displayTeamMembers.length).toBe(2); + }); + + it('should render the learner ownership chip before the selected member', () => { + component.question = { + teamMembers: [ + { key: 'member-1', userName: 'Member 1' }, + { key: 'member-2', userName: 'Member 2' }, + ], + audience: ['submitter', 'reviewer'], + }; + component.submissionStatus = 'feedback available'; + component.reviewStatus = 'done'; + component.doAssessment = false; + component.doReview = false; + component.viewerRole = 'learner'; + component.submission = { answer: 'member-1' }; + component.review = { answer: 'member-2' }; + + fixture.detectChanges(); + + const learnerItem = fixture.nativeElement.querySelector('ion-list ion-item'); + const labelChildren = Array.from(learnerItem.querySelector('ion-label').children); + expect(labelChildren.indexOf(learnerItem.querySelector('p'))) + .toBeLessThan(labelChildren.indexOf(learnerItem.querySelector('.answer-content'))); + expect(learnerItem.textContent).toContain('Your Answer'); + }); + describe('when testing onInit()', () => { it('should get correct data for in progress submission', () => { component.question = { diff --git a/projects/v3/src/app/components/team-member-selector/team-member-selector.component.ts b/projects/v3/src/app/components/team-member-selector/team-member-selector.component.ts index 1f27b232f6..1334f95d8f 100644 --- a/projects/v3/src/app/components/team-member-selector/team-member-selector.component.ts +++ b/projects/v3/src/app/components/team-member-selector/team-member-selector.component.ts @@ -31,6 +31,8 @@ export class TeamMemberSelectorComponent implements ControlValueAccessor, OnInit @Input() doAssessment: Boolean; // this is for doing review or not @Input() doReview: Boolean; + @Input() viewerRole: 'learner' | 'reviewer'; + @Input() isReviewerFeedbackContext = false; // FormControl that is passed in from parent component @Input() control: AbstractControl; // answer field for submitter & reviewer @@ -180,6 +182,15 @@ export class TeamMemberSelectorComponent implements ControlValueAccessor, OnInit return !this.doAssessment && !this.doReview && (this.submissionStatus === 'feedback available' || this.submissionStatus === 'pending review' || (this.submissionStatus === 'done' && this.reviewStatus === '')) && (this.submission?.answer || this.review?.answer); } + get displayTeamMembers(): Array { + const teamMembers = this.question?.teamMembers || []; + if (!this.isReviewerFeedbackContext) { + return teamMembers; + } + + return teamMembers.filter(teamMember => teamMember.key === this.review?.answer); + } + // innerHTML text toggle - submission onLabelToggle = (id: string): void => { this.onChange(id); diff --git a/projects/v3/src/app/components/text/text.component.html b/projects/v3/src/app/components/text/text.component.html index b33fb38eeb..8cb0b64b76 100644 --- a/projects/v3/src/app/components/text/text.component.html +++ b/projects/v3/src/app/components/text/text.component.html @@ -2,7 +2,12 @@

{{question.n
-

Learner's Answer

+

+ + Your Answer + Learner's Answer + +

@@ -10,7 +15,7 @@

{{question.n -

Reviewer's Answer

+

Reviewer's Answer

 diff --git a/projects/v3/src/app/components/text/text.component.spec.ts b/projects/v3/src/app/components/text/text.component.spec.ts index 6700e149d4..79f7215e28 100644 --- a/projects/v3/src/app/components/text/text.component.spec.ts +++ b/projects/v3/src/app/components/text/text.component.spec.ts @@ -49,6 +49,50 @@ describe('TextComponent', () => { expect(component).toBeDefined(); }); + describe('read-only ownership context', () => { + beforeEach(() => { + component.question = { + id: 1, + name: 'Question', + type: 'text', + description: '', + isRequired: false, + canAnswer: true, + canComment: false, + audience: ['submitter', 'reviewer'], + } as any; + component.submissionStatus = 'feedback available'; + component.reviewStatus = 'done'; + component.doAssessment = false; + component.doReview = false; + component.submission = { answer: 'learner response' }; + component.review = { answer: 'reviewer response' }; + }); + + it('should use ownership labels for shared feedback', () => { + component.viewerRole = 'learner'; + + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toContain('Your Answer'); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + }); + + it('should show reviewer-only text without an ownership label', () => { + component.question.audience = ['reviewer']; + component.question.reviewerOnly = true; + component.submission = {}; + component.isReviewerFeedbackContext = true; + + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toContain('reviewer response'); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); + expect(fixture.nativeElement.querySelectorAll('ion-chip').length).toBe(0); + }); + }); + describe('when testing onInit()', () => { const dummyQuestion = { id: 1, @@ -576,4 +620,3 @@ describe('TextComponent', () => { }); }); - diff --git a/projects/v3/src/app/components/text/text.component.ts b/projects/v3/src/app/components/text/text.component.ts index b3c7cac3c9..069f8cffe7 100644 --- a/projects/v3/src/app/components/text/text.component.ts +++ b/projects/v3/src/app/components/text/text.component.ts @@ -31,6 +31,8 @@ export class TextComponent implements ControlValueAccessor, OnInit, AfterViewIni @Input() submissionStatus; @Input() doAssessment: Boolean; @Input() doReview: Boolean; + @Input() viewerRole: 'learner' | 'reviewer'; + @Input() isReviewerFeedbackContext = false; @Input() control: AbstractControl; // answer field for submitter & reviewer From 558c3b797f10edd88f630e319d338c089de3c339 Mon Sep 17 00:00:00 2001 From: trtshen Date: Fri, 17 Jul 2026 14:37:20 +0800 Subject: [PATCH 03/14] [CORE-8277] reivewer-only question improvement --- docs/assessment-flow.md | 4 +- .../multiple/multiple.component.html | 27 +++-- .../multiple/multiple.component.spec.ts | 82 +++++++++++++ .../components/multiple/multiple.component.ts | 16 ++- .../app/components/oneof/oneof.component.html | 23 +++- .../components/oneof/oneof.component.spec.ts | 109 ++++++++++++++++++ .../app/components/oneof/oneof.component.ts | 13 ++- 7 files changed, 257 insertions(+), 17 deletions(-) diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index 4158a91111..a3f48f2aef 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -2,7 +2,7 @@ status: stable authority: canonical scope: frontend -last_reviewed: 2026-07-13 +last_reviewed: 2026-07-16 supersedes: none --- @@ -135,6 +135,8 @@ questionsForm: FormGroup = new FormGroup({}); 3. **Feedback Available**: Read-only with feedback - Display learner answers and reviewer feedback + - For reviewer-only `multiple` and `oneof` questions, display every configured choice and label it as either **Selected by reviewer** or **Not selected by reviewer** + - Reviewer-only choice feedback is derived only from the review answer and does not display learner-answer labels - "Mark as Read" button to acknowledge feedback - Navigation to next task after reading diff --git a/projects/v3/src/app/components/multiple/multiple.component.html b/projects/v3/src/app/components/multiple/multiple.component.html index 0a72dc4579..0e713590d6 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.html +++ b/projects/v3/src/app/components/multiple/multiple.component.html @@ -6,14 +6,25 @@

{ [ngClass]="{'item-bottom-border': !(choice.explanation && choice.explanation.changingThisBreaksApplicationSecurity && submission?.answer?.includes(choice.id))}">
- Learner's Answer - Reviewer's Answer + + Selected by reviewer + + Not selected by reviewer + + + + Learner's Answer + Reviewer's Answer + diff --git a/projects/v3/src/app/components/multiple/multiple.component.spec.ts b/projects/v3/src/app/components/multiple/multiple.component.spec.ts index a0685aa4eb..5194ea25fa 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.spec.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.spec.ts @@ -178,6 +178,88 @@ describe('MultipleComponent', () => { expect(fixture.nativeElement.textContent).not.toContain('choice1'); expect(fixture.nativeElement.textContent).not.toContain('choice3'); }); + + it('should show every reviewer-only choice with its reviewer selection state to the learner', () => { + component.question = { + reviewerOnly: true, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = { answer: [1, 3] }; + component.review = { answer: [2] }; + + fixture.detectChanges(); + + const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); + const selectedChips = fixture.nativeElement.querySelectorAll('ion-chip.success'); + const notSelectedChips = fixture.nativeElement.querySelectorAll('ion-chip.orange'); + + expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); + expect(items.length).toBe(3); + expect(selectedChips.length).toBe(1); + expect(selectedChips[0].textContent.trim()).toBe('Selected by reviewer'); + expect(notSelectedChips.length).toBe(2); + expect(Array.from(notSelectedChips).every((chip: Element) => chip.textContent.trim() === 'Not selected by reviewer')).toBeTrue(); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); + }); + + it('should show every reviewer-only choice as not selected when the review answer is empty', () => { + component.question = { + reviewerOnly: true, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = {}; + component.review = { answer: [] }; + + fixture.detectChanges(); + + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); + expect(fixture.nativeElement.querySelectorAll('ion-chip.success').length).toBe(0); + expect(fixture.nativeElement.querySelectorAll('ion-chip.orange').length).toBe(2); + }); + + it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { + component.question = { + reviewerOnly: true, + canAnswer: true, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = {}; + component.review = { answer: [2] }; + + fixture.detectChanges(); + + expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([2]); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); + expect(fixture.nativeElement.textContent).not.toContain('Not selected by reviewer'); + }); }); it('when testing writeValue(), it should pass data correctly', () => { diff --git a/projects/v3/src/app/components/multiple/multiple.component.ts b/projects/v3/src/app/components/multiple/multiple.component.ts index 8cd087be04..5e1a037929 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.ts @@ -247,8 +247,15 @@ export class MultipleComponent implements AfterViewInit, ControlValueAccessor, O return !this.doAssessment && !this.doReview && (this.submissionStatus === 'feedback available' || this.submissionStatus === 'pending review' || (this.submissionStatus === 'done' && this.reviewStatus === '')); } + get isReviewerOnlyLearnerFeedback(): boolean { + return this.isDisplayOnly + && this.question?.reviewerOnly === true + && this.question?.canAnswer === false + && this.submissionStatus === 'feedback available'; + } + get displayChoices(): Array { - if (!this.isDisplayOnly) { + if (!this.isDisplayOnly || this.isReviewerOnlyLearnerFeedback) { return this.question?.choices || []; } @@ -259,6 +266,13 @@ export class MultipleComponent implements AfterViewInit, ControlValueAccessor, O return (this.question?.choices || []).filter(choice => selectedChoiceIds.has(choice.id)); } + isReviewChoiceSelected(choiceId: string | number): boolean { + const selectedChoiceIds = new Set(); + this._collectSelectedChoiceIds(this.review?.answer, selectedChoiceIds); + + return selectedChoiceIds.has(choiceId); + } + private _collectSelectedChoiceIds(answer: any, selectedChoiceIds: Set): void { if (answer === null || answer === undefined) { return; diff --git a/projects/v3/src/app/components/oneof/oneof.component.html b/projects/v3/src/app/components/oneof/oneof.component.html index 53ba85301a..d56b65af60 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.html +++ b/projects/v3/src/app/components/oneof/oneof.component.html @@ -7,12 +7,23 @@

{{question.
- Learner's Answer - Reviewer's Answer + + Selected by reviewer + + Not selected by reviewer + + + + Learner's Answer + Reviewer's Answer + diff --git a/projects/v3/src/app/components/oneof/oneof.component.spec.ts b/projects/v3/src/app/components/oneof/oneof.component.spec.ts index 4d63ed46eb..87c32533fe 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.spec.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.spec.ts @@ -176,6 +176,115 @@ describe('OneofComponent', () => { }); }); + describe('when testing display-only preview mode', () => { + it('should show every reviewer-only choice with its reviewer selection state to the learner', () => { + component.question = { + reviewerOnly: true, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = { answer: 1 }; + component.review = { answer: 2 }; + + fixture.detectChanges(); + + const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); + const selectedChips = fixture.nativeElement.querySelectorAll('ion-chip.success'); + const notSelectedChips = fixture.nativeElement.querySelectorAll('ion-chip.orange'); + + expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); + expect(items.length).toBe(3); + expect(selectedChips.length).toBe(1); + expect(selectedChips[0].textContent.trim()).toBe('Selected by reviewer'); + expect(notSelectedChips.length).toBe(2); + expect(Array.from(notSelectedChips).every((chip: Element) => chip.textContent.trim() === 'Not selected by reviewer')).toBeTrue(); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); + }); + + it('should show every reviewer-only choice as not selected when the review answer is empty', () => { + component.question = { + reviewerOnly: true, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = {}; + component.review = {}; + + fixture.detectChanges(); + + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); + expect(fixture.nativeElement.querySelectorAll('ion-chip.success').length).toBe(0); + expect(fixture.nativeElement.querySelectorAll('ion-chip.orange').length).toBe(2); + }); + + it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { + component.question = { + reviewerOnly: true, + canAnswer: true, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = {}; + component.review = { answer: 2 }; + + fixture.detectChanges(); + + expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([2]); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); + expect(fixture.nativeElement.textContent).not.toContain('Not selected by reviewer'); + }); + + it('should preserve selected-only rendering for shared-audience questions', () => { + component.question = { + reviewerOnly: false, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['submitter', 'reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = { answer: 1 }; + component.review = { answer: 2 }; + + fixture.detectChanges(); + + expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); + expect(fixture.nativeElement.textContent).toContain("Learner's Answer"); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); + }); + }); + describe('triggerSave()', () => { beforeEach(() => { component.question = { id: 42, audience: [] }; diff --git a/projects/v3/src/app/components/oneof/oneof.component.ts b/projects/v3/src/app/components/oneof/oneof.component.ts index 132d577f99..87a7e727a4 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.ts @@ -210,8 +210,15 @@ export class OneofComponent implements AfterViewInit, ControlValueAccessor, OnIn return !this.doAssessment && !this.doReview && (this.submissionStatus === 'feedback available' || this.submissionStatus === 'pending review' || (this.submissionStatus === 'done' && this.reviewStatus === '')); } + get isReviewerOnlyLearnerFeedback(): boolean { + return this.isDisplayOnly + && this.question?.reviewerOnly === true + && this.question?.canAnswer === false + && this.submissionStatus === 'feedback available'; + } + get displayChoices(): Array { - if (!this.isDisplayOnly) { + if (!this.isDisplayOnly || this.isReviewerOnlyLearnerFeedback) { return this.question?.choices || []; } @@ -230,6 +237,10 @@ export class OneofComponent implements AfterViewInit, ControlValueAccessor, OnIn return (this.question?.choices || []).filter(choice => selectedIds.has(choice.id)); } + isReviewChoiceSelected(choiceId: string | number): boolean { + return this.review?.answer === choiceId; + } + // innerHTML text toggle onLabelToggle = (id: string): void => { this.onChange(id); From de794ebdad6edf3edabc5ac86ca18b088f27eeb1 Mon Sep 17 00:00:00 2001 From: trtshen Date: Tue, 21 Jul 2026 12:22:10 +0800 Subject: [PATCH 04/14] [CORE-8277] Enhance reviewer-only question feedback display --- docs/assessment-flow.md | 2 +- .../multiple/multiple.component.html | 29 +++++++++--- .../multiple/multiple.component.scss | 44 ++++++++++++++++++ .../multiple/multiple.component.spec.ts | 20 +++++---- .../app/components/oneof/oneof.component.html | 29 +++++++++--- .../app/components/oneof/oneof.component.scss | 45 +++++++++++++++++++ .../components/oneof/oneof.component.spec.ts | 20 +++++---- 7 files changed, 158 insertions(+), 31 deletions(-) diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index a3f48f2aef..dfe7709cdf 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -135,7 +135,7 @@ questionsForm: FormGroup = new FormGroup({}); 3. **Feedback Available**: Read-only with feedback - Display learner answers and reviewer feedback - - For reviewer-only `multiple` and `oneof` questions, display every configured choice and label it as either **Selected by reviewer** or **Not selected by reviewer** + - For reviewer-only `multiple` and `oneof` questions, display every configured choice with a green check and **Selected** status or a subdued **Not selected** status - Reviewer-only choice feedback is derived only from the review answer and does not display learner-answer labels - "Mark as Read" button to acknowledge feedback - Navigation to next task after reading diff --git a/projects/v3/src/app/components/multiple/multiple.component.html b/projects/v3/src/app/components/multiple/multiple.component.html index 0e713590d6..db84e3e8f9 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.html +++ b/projects/v3/src/app/components/multiple/multiple.component.html @@ -4,15 +4,30 @@

{ - -
+ +
- Selected by reviewer + + + + Selected by reviewer + - Not selected by reviewer + + + Not selected by reviewer + diff --git a/projects/v3/src/app/components/multiple/multiple.component.scss b/projects/v3/src/app/components/multiple/multiple.component.scss index 46fc455458..0f02e2c75f 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.scss +++ b/projects/v3/src/app/components/multiple/multiple.component.scss @@ -65,3 +65,47 @@ ion-item { --min-height: 1em; } +ion-label.reviewer-feedback-choice { + display: flex !important; + align-items: center; + justify-content: space-between; + gap: 16px; +} + +.reviewer-feedback-choice-text { + flex: 1 1 auto; + min-width: 0; +} + +.reviewer-choice-status { + display: inline-flex; + flex: 0 0 auto; + align-items: center; + gap: 4px; + font-size: 14px; + line-height: 20px; + white-space: nowrap; +} + +.reviewer-choice-status-selected { + color: var(--ion-color-success-shade); + font-weight: 700; + + ion-icon { + --ionicon-stroke-width: 48px; + font-size: 18px; + } +} + +.reviewer-choice-status-not-selected { + color: var(--practera-grey-50); + font-weight: 400; +} + +@media (max-width: 576px) { + ion-label.reviewer-feedback-choice { + align-items: flex-start; + flex-direction: column; + gap: 8px; + } +} diff --git a/projects/v3/src/app/components/multiple/multiple.component.spec.ts b/projects/v3/src/app/components/multiple/multiple.component.spec.ts index 5194ea25fa..36c01aa2f3 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.spec.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.spec.ts @@ -199,16 +199,20 @@ describe('MultipleComponent', () => { fixture.detectChanges(); const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); - const selectedChips = fixture.nativeElement.querySelectorAll('ion-chip.success'); - const notSelectedChips = fixture.nativeElement.querySelectorAll('ion-chip.orange'); + const selectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected'); + const notSelectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected'); expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); expect(items.length).toBe(3); - expect(selectedChips.length).toBe(1); - expect(selectedChips[0].textContent.trim()).toBe('Selected by reviewer'); - expect(notSelectedChips.length).toBe(2); - expect(Array.from(notSelectedChips).every((chip: Element) => chip.textContent.trim() === 'Not selected by reviewer')).toBeTrue(); + expect(selectedStatuses.length).toBe(1); + expect(selectedStatuses[0].querySelector('ion-icon').getAttribute('name')).toBe('checkmark'); + expect(selectedStatuses[0].querySelector('.reviewer-choice-status-text').textContent.trim()).toBe('Selected'); + expect(selectedStatuses[0].textContent).toContain('Selected by reviewer'); + expect(notSelectedStatuses.length).toBe(2); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.querySelector('ion-icon') === null)).toBeTrue(); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.querySelector('.reviewer-choice-status-text').textContent.trim() === 'Not selected')).toBeTrue(); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.textContent.includes('Not selected by reviewer'))).toBeTrue(); expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); }); @@ -232,8 +236,8 @@ describe('MultipleComponent', () => { fixture.detectChanges(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); - expect(fixture.nativeElement.querySelectorAll('ion-chip.success').length).toBe(0); - expect(fixture.nativeElement.querySelectorAll('ion-chip.orange').length).toBe(2); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected').length).toBe(0); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected').length).toBe(2); }); it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { diff --git a/projects/v3/src/app/components/oneof/oneof.component.html b/projects/v3/src/app/components/oneof/oneof.component.html index d56b65af60..3036dee480 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.html +++ b/projects/v3/src/app/components/oneof/oneof.component.html @@ -5,15 +5,30 @@

{{question. - -
+ +
- Selected by reviewer + + + + Selected by reviewer + - Not selected by reviewer + + + Not selected by reviewer + diff --git a/projects/v3/src/app/components/oneof/oneof.component.scss b/projects/v3/src/app/components/oneof/oneof.component.scss index c710298e38..2242e53ca5 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.scss +++ b/projects/v3/src/app/components/oneof/oneof.component.scss @@ -70,3 +70,48 @@ ion-item { .feedback-title { --min-height: 1em; } + +ion-label.reviewer-feedback-choice { + display: flex !important; + align-items: center; + justify-content: space-between; + gap: 16px; +} + +.reviewer-feedback-choice-text { + flex: 1 1 auto; + min-width: 0; +} + +.reviewer-choice-status { + display: inline-flex; + flex: 0 0 auto; + align-items: center; + gap: 4px; + font-size: 14px; + line-height: 20px; + white-space: nowrap; +} + +.reviewer-choice-status-selected { + color: var(--ion-color-success-shade); + font-weight: 700; + + ion-icon { + --ionicon-stroke-width: 48px; + font-size: 18px; + } +} + +.reviewer-choice-status-not-selected { + color: var(--practera-grey-50); + font-weight: 400; +} + +@media (max-width: 576px) { + ion-label.reviewer-feedback-choice { + align-items: flex-start; + flex-direction: column; + gap: 8px; + } +} diff --git a/projects/v3/src/app/components/oneof/oneof.component.spec.ts b/projects/v3/src/app/components/oneof/oneof.component.spec.ts index 87c32533fe..ed31ff3a76 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.spec.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.spec.ts @@ -197,16 +197,20 @@ describe('OneofComponent', () => { fixture.detectChanges(); const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); - const selectedChips = fixture.nativeElement.querySelectorAll('ion-chip.success'); - const notSelectedChips = fixture.nativeElement.querySelectorAll('ion-chip.orange'); + const selectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected'); + const notSelectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected'); expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); expect(items.length).toBe(3); - expect(selectedChips.length).toBe(1); - expect(selectedChips[0].textContent.trim()).toBe('Selected by reviewer'); - expect(notSelectedChips.length).toBe(2); - expect(Array.from(notSelectedChips).every((chip: Element) => chip.textContent.trim() === 'Not selected by reviewer')).toBeTrue(); + expect(selectedStatuses.length).toBe(1); + expect(selectedStatuses[0].querySelector('ion-icon').getAttribute('name')).toBe('checkmark'); + expect(selectedStatuses[0].querySelector('.reviewer-choice-status-text').textContent.trim()).toBe('Selected'); + expect(selectedStatuses[0].textContent).toContain('Selected by reviewer'); + expect(notSelectedStatuses.length).toBe(2); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.querySelector('ion-icon') === null)).toBeTrue(); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.querySelector('.reviewer-choice-status-text').textContent.trim() === 'Not selected')).toBeTrue(); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.textContent.includes('Not selected by reviewer'))).toBeTrue(); expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); }); @@ -230,8 +234,8 @@ describe('OneofComponent', () => { fixture.detectChanges(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); - expect(fixture.nativeElement.querySelectorAll('ion-chip.success').length).toBe(0); - expect(fixture.nativeElement.querySelectorAll('ion-chip.orange').length).toBe(2); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected').length).toBe(0); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected').length).toBe(2); }); it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { From 5c08cb75d3a465979fd71edf5ee48219bfdc6e40 Mon Sep 17 00:00:00 2001 From: trtshen Date: Tue, 21 Jul 2026 13:51:20 +0800 Subject: [PATCH 05/14] [CORE-8282] improved premature button re-enable for review submission --- docs/assessment-flow.md | 14 ++++- .../assessment/assessment.component.html | 1 + .../assessment/assessment.component.spec.ts | 52 ++++++++++++++++ .../assessment/assessment.component.ts | 11 +++- .../bottom-action-bar.component.html | 16 ++++- .../bottom-action-bar.component.scss | 8 ++- .../bottom-action-bar.component.spec.ts | 61 +++++++++++++++++++ .../bottom-action-bar.component.ts | 36 +++++++++-- 8 files changed, 185 insertions(+), 14 deletions(-) diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index dfe7709cdf..548ac23e56 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -302,17 +302,25 @@ All follow similar patterns with dual-purpose display for learner/reviewer conte ```html {{ text }} + [attr.aria-busy]="loading ? 'true' : 'false'"> + + {{ text }} + ``` **Button States:** - **Enabled**: Form is valid and user can submit -- **Disabled**: Form has validation errors or submission in progress +- **Disabled**: Form has validation errors or an action is already in progress +- **Loading**: For assessment/review submit actions, starts before the click event is emitted, keeps the disabled button visible with an inline spinner, and clears when `disabled$` emits `false` - **Dynamic Text**: Changes based on context (Submit, Continue, Mark as Read, etc.) +`disabled$` remains the source of truth for whether the action can be triggered. Loading is a distinct, opt-in visual state (`showLoadingOnClick`) so validation-disabled buttons do not incorrectly announce `aria-busy`, and non-submit actions retain their existing behavior. + +During manual submission, the parent page owns the terminal `disabled$ = false` transition. Intermediate assessment/review refetches may update displayed data and the last-saved message, but must not re-enable the action while the assessment component's submission guard is active. The parent clears the state only after the final refresh succeeds or the submission fails. + ## Data Flow Diagrams ### Assessment Submission Flow (Learner) diff --git a/projects/v3/src/app/components/assessment/assessment.component.html b/projects/v3/src/app/components/assessment/assessment.component.html index 4325d5f425..a290536af5 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.html +++ b/projects/v3/src/app/components/assessment/assessment.component.html @@ -443,6 +443,7 @@ (handleResubmit)="resubmit()" [text]="btnText" [disabled$]="btnDisabled$" + [showLoadingOnClick]="showSubmitLoadingOnClick" (handleClick)="continueToNextTask()" [hasCustomContent]="isPaginationEnabled && pageCount > 1"> diff --git a/projects/v3/src/app/components/assessment/assessment.component.spec.ts b/projects/v3/src/app/components/assessment/assessment.component.spec.ts index c6bf94fc06..048b94e69a 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.spec.ts +++ b/projects/v3/src/app/components/assessment/assessment.component.spec.ts @@ -1227,6 +1227,20 @@ describe('AssessmentComponent', () => { }); describe('continueToNextTask()', () => { + it('should enable loading-on-click only for submit actions', () => { + component.doAssessment = true; + component.isPendingReview = false; + expect(component.showSubmitLoadingOnClick).toBeTrue(); + + component.doAssessment = false; + component.isPendingReview = true; + expect(component.showSubmitLoadingOnClick).toBeTrue(); + + component.isPendingReview = false; + component.submission = { ...mockSubmission, status: 'done' } as any; + expect(component.showSubmitLoadingOnClick).toBeFalse(); + }); + it('should submit assessment', async () => { component.doAssessment = true; expect(component.btnText).toEqual('submit answers'); @@ -1658,6 +1672,44 @@ describe('AssessmentComponent', () => { }); describe('ngOnChanges() submitting flag preservation', () => { + it('should keep the review button disabled when an in-progress review is refetched during submit', () => { + component.action = 'review'; + component.assessment = { ...mockAssessment, type: 'moderated' } as any; + component.submission = { ...mockSubmission, status: 'pending review' } as any; + component.review = { ...mockReview, status: 'in progress' } as any; + component['submitting'] = true; + component.btnDisabled$.next(true); + + component.ngOnChanges({ + submission: { + previousValue: component.submission, + currentValue: component.submission, + firstChange: false, + isFirstChange: () => false, + }, + review: { + previousValue: component.review, + currentValue: component.review, + firstChange: false, + isFirstChange: () => false, + }, + } as any); + + expect(component['submitting']).toBeTrue(); + expect(component.btnDisabled$.getValue()).toBeTrue(); + }); + + it('should enable the review button when an in-progress review loads outside submission', () => { + component.isPendingReview = true; + component.review = { ...mockReview, status: 'in progress' } as any; + component['submitting'] = false; + component.btnDisabled$.next(true); + + component['_handleReviewData'](); + + expect(component.btnDisabled$.getValue()).toBeFalse(); + }); + it('should preserve submitting=true when same submission is refetched during submit', () => { // simulate initial state: user clicked submit component.ngOnChanges({ diff --git a/projects/v3/src/app/components/assessment/assessment.component.ts b/projects/v3/src/app/components/assessment/assessment.component.ts index d808bd1a63..dcd9f99e35 100644 --- a/projects/v3/src/app/components/assessment/assessment.component.ts +++ b/projects/v3/src/app/components/assessment/assessment.component.ts @@ -708,7 +708,12 @@ Best regards`; private _handleReviewData() { if (this.isPendingReview && this.review?.status === 'in progress') { this.savingMessage$.next($localize`Last saved ${this.utils.timeFormatter(this.review.modified)}`); - this.btnDisabled$.next(false); + // An intermediate status-check fetch republishes the same in-progress review while the + // submit request is still running. Keep the action disabled until the parent submission + // workflow explicitly reports completion or failure. + if (!this.submitting) { + this.btnDisabled$.next(false); + } } } @@ -989,6 +994,10 @@ Best regards`; return 'continue'; } + get showSubmitLoadingOnClick(): boolean { + return this._btnAction === 'submit'; + } + // the text of the button get btnText() { switch (this._btnAction) { diff --git a/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.html b/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.html index d4e86a950b..3795bbd3b3 100644 --- a/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.html +++ b/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.html @@ -11,15 +11,25 @@
{{ text }} + > + + {{ text }} + { beforeEach(async () => { await TestBed.configureTestingModule({ declarations: [BottomActionBarComponent], + schemas: [CUSTOM_ELEMENTS_SCHEMA], }).compileComponents(); }); @@ -31,6 +33,8 @@ describe('BottomActionBarComponent', () => { expect(component.buttonType).toBe(''); expect(component.hasCustomContent).toBe(false); expect(component.disabled$).toBeUndefined(); + expect(component.showLoadingOnClick).toBe(false); + expect(component.loading).toBe(false); }); }); @@ -88,6 +92,63 @@ describe('BottomActionBarComponent', () => { expect(component.handleClick.emit).toHaveBeenCalledWith(clickEvent); }); + + it('should show loading immediately and prevent duplicate clicks when opted in', () => { + const disabled$ = new BehaviorSubject(false); + fixture.componentRef.setInput('disabled$', disabled$); + fixture.componentRef.setInput('showLoadingOnClick', true); + fixture.detectChanges(); + spyOn(component.handleClick, 'emit'); + + const clickEvent = new MouseEvent('click'); + component.onClick(clickEvent); + component.onClick(clickEvent); + fixture.detectChanges(); + + expect(component.loading).toBeTrue(); + expect(component.handleClick.emit).toHaveBeenCalledTimes(1); + expect(fixture.debugElement.query(By.css('ion-spinner.action-spinner'))).toBeTruthy(); + expect(fixture.debugElement.query(By.css('.button-container.is-loading'))).toBeTruthy(); + + const actionButton = fixture.debugElement.query(By.css('ion-button.action-button')); + expect(actionButton.properties.disabled).toBeTrue(); + expect(actionButton.attributes['aria-busy']).toBe('true'); + }); + + it('should clear loading when disabled$ emits false after processing', () => { + const disabled$ = new BehaviorSubject(false); + fixture.componentRef.setInput('disabled$', disabled$); + fixture.componentRef.setInput('showLoadingOnClick', true); + fixture.detectChanges(); + + component.onClick(new MouseEvent('click')); + disabled$.next(true); + expect(component.loading).toBeTrue(); + + disabled$.next(false); + fixture.detectChanges(); + + expect(component.loading).toBeFalse(); + expect(fixture.debugElement.query(By.css('ion-spinner.action-spinner'))).toBeNull(); + }); + + it('should not enter loading or emit when already disabled', () => { + fixture.componentRef.setInput('disabled$', new BehaviorSubject(true)); + fixture.componentRef.setInput('showLoadingOnClick', true); + fixture.detectChanges(); + spyOn(component.handleClick, 'emit'); + + component.onClick(new MouseEvent('click')); + fixture.detectChanges(); + + expect(component.loading).toBeFalse(); + expect(component.handleClick.emit).not.toHaveBeenCalled(); + expect(fixture.debugElement.query(By.css('ion-spinner.action-spinner'))).toBeNull(); + + const actionButton = fixture.debugElement.query(By.css('ion-button.action-button')); + expect(actionButton.properties.disabled).toBeTrue(); + expect(actionButton.attributes['aria-busy']).toBe('false'); + }); }); describe('onResubmit()', () => { diff --git a/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.ts b/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.ts index e1b8df616c..37c9750c6b 100644 --- a/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.ts +++ b/projects/v3/src/app/components/bottom-action-bar/bottom-action-bar.component.ts @@ -1,5 +1,5 @@ -import { Component, Input, Output, EventEmitter, OnChanges } from '@angular/core'; -import { BehaviorSubject } from 'rxjs'; +import { Component, Input, Output, EventEmitter, OnChanges, OnDestroy, SimpleChanges } from '@angular/core'; +import { BehaviorSubject, Subscription } from 'rxjs'; @Component({ standalone: false, @@ -7,26 +7,51 @@ import { BehaviorSubject } from 'rxjs'; templateUrl: 'bottom-action-bar.component.html', styleUrls: ['./bottom-action-bar.component.scss'], }) -export class BottomActionBarComponent { +export class BottomActionBarComponent implements OnChanges, OnDestroy { @Input() showResubmit: boolean = false; @Input() text: string; @Input() color: string = 'primary'; @Input() disabled$?: BehaviorSubject; // assessment only + @Input() showLoadingOnClick: boolean = false; @Output() handleClick = new EventEmitter(); @Output() handleResubmit = new EventEmitter(); @Input() buttonType: string = ''; @Input() hasCustomContent: boolean = false; + loading = false; + + private disabledSubscription?: Subscription; + constructor() {} + ngOnChanges(changes: SimpleChanges): void { + if (!changes.disabled$) { + return; + } + + this.disabledSubscription?.unsubscribe(); + this.disabledSubscription = this.disabled$?.subscribe(disabled => { + if (disabled === false) { + this.loading = false; + } + }); + } + + ngOnDestroy(): void { + this.disabledSubscription?.unsubscribe(); + } + onClick(clickEvent: Event) { - // if disabled, do nothing - if (this.disabled$?.getValue() === true) { + // if disabled or already processing, do nothing + if (this.disabled$?.getValue() === true || this.loading) { return; } // make sure it's the click event that triggers "handleClick" if (clickEvent.type === 'click') { + if (this.showLoadingOnClick) { + this.loading = true; + } return this.handleClick.emit(clickEvent); } @@ -37,4 +62,3 @@ export class BottomActionBarComponent { return this.handleResubmit.emit(clickEvent); } } - From 51b3c49d98d81c004c96d564a4fddc56914aa030 Mon Sep 17 00:00:00 2001 From: trtshen Date: Fri, 17 Jul 2026 14:37:20 +0800 Subject: [PATCH 06/14] [CORE-8277] reivewer-only question improvement --- docs/assessment-flow.md | 4 +- .../multiple/multiple.component.html | 27 +++-- .../multiple/multiple.component.spec.ts | 82 +++++++++++++ .../components/multiple/multiple.component.ts | 16 ++- .../app/components/oneof/oneof.component.html | 23 +++- .../components/oneof/oneof.component.spec.ts | 109 ++++++++++++++++++ .../app/components/oneof/oneof.component.ts | 13 ++- 7 files changed, 257 insertions(+), 17 deletions(-) diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index 4158a91111..a3f48f2aef 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -2,7 +2,7 @@ status: stable authority: canonical scope: frontend -last_reviewed: 2026-07-13 +last_reviewed: 2026-07-16 supersedes: none --- @@ -135,6 +135,8 @@ questionsForm: FormGroup = new FormGroup({}); 3. **Feedback Available**: Read-only with feedback - Display learner answers and reviewer feedback + - For reviewer-only `multiple` and `oneof` questions, display every configured choice and label it as either **Selected by reviewer** or **Not selected by reviewer** + - Reviewer-only choice feedback is derived only from the review answer and does not display learner-answer labels - "Mark as Read" button to acknowledge feedback - Navigation to next task after reading diff --git a/projects/v3/src/app/components/multiple/multiple.component.html b/projects/v3/src/app/components/multiple/multiple.component.html index 0a72dc4579..0e713590d6 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.html +++ b/projects/v3/src/app/components/multiple/multiple.component.html @@ -6,14 +6,25 @@

{ [ngClass]="{'item-bottom-border': !(choice.explanation && choice.explanation.changingThisBreaksApplicationSecurity && submission?.answer?.includes(choice.id))}">
- Learner's Answer - Reviewer's Answer + + Selected by reviewer + + Not selected by reviewer + + + + Learner's Answer + Reviewer's Answer + diff --git a/projects/v3/src/app/components/multiple/multiple.component.spec.ts b/projects/v3/src/app/components/multiple/multiple.component.spec.ts index a0685aa4eb..5194ea25fa 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.spec.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.spec.ts @@ -178,6 +178,88 @@ describe('MultipleComponent', () => { expect(fixture.nativeElement.textContent).not.toContain('choice1'); expect(fixture.nativeElement.textContent).not.toContain('choice3'); }); + + it('should show every reviewer-only choice with its reviewer selection state to the learner', () => { + component.question = { + reviewerOnly: true, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = { answer: [1, 3] }; + component.review = { answer: [2] }; + + fixture.detectChanges(); + + const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); + const selectedChips = fixture.nativeElement.querySelectorAll('ion-chip.success'); + const notSelectedChips = fixture.nativeElement.querySelectorAll('ion-chip.orange'); + + expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); + expect(items.length).toBe(3); + expect(selectedChips.length).toBe(1); + expect(selectedChips[0].textContent.trim()).toBe('Selected by reviewer'); + expect(notSelectedChips.length).toBe(2); + expect(Array.from(notSelectedChips).every((chip: Element) => chip.textContent.trim() === 'Not selected by reviewer')).toBeTrue(); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); + }); + + it('should show every reviewer-only choice as not selected when the review answer is empty', () => { + component.question = { + reviewerOnly: true, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = {}; + component.review = { answer: [] }; + + fixture.detectChanges(); + + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); + expect(fixture.nativeElement.querySelectorAll('ion-chip.success').length).toBe(0); + expect(fixture.nativeElement.querySelectorAll('ion-chip.orange').length).toBe(2); + }); + + it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { + component.question = { + reviewerOnly: true, + canAnswer: true, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = {}; + component.review = { answer: [2] }; + + fixture.detectChanges(); + + expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([2]); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); + expect(fixture.nativeElement.textContent).not.toContain('Not selected by reviewer'); + }); }); it('when testing writeValue(), it should pass data correctly', () => { diff --git a/projects/v3/src/app/components/multiple/multiple.component.ts b/projects/v3/src/app/components/multiple/multiple.component.ts index 8cd087be04..5e1a037929 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.ts @@ -247,8 +247,15 @@ export class MultipleComponent implements AfterViewInit, ControlValueAccessor, O return !this.doAssessment && !this.doReview && (this.submissionStatus === 'feedback available' || this.submissionStatus === 'pending review' || (this.submissionStatus === 'done' && this.reviewStatus === '')); } + get isReviewerOnlyLearnerFeedback(): boolean { + return this.isDisplayOnly + && this.question?.reviewerOnly === true + && this.question?.canAnswer === false + && this.submissionStatus === 'feedback available'; + } + get displayChoices(): Array { - if (!this.isDisplayOnly) { + if (!this.isDisplayOnly || this.isReviewerOnlyLearnerFeedback) { return this.question?.choices || []; } @@ -259,6 +266,13 @@ export class MultipleComponent implements AfterViewInit, ControlValueAccessor, O return (this.question?.choices || []).filter(choice => selectedChoiceIds.has(choice.id)); } + isReviewChoiceSelected(choiceId: string | number): boolean { + const selectedChoiceIds = new Set(); + this._collectSelectedChoiceIds(this.review?.answer, selectedChoiceIds); + + return selectedChoiceIds.has(choiceId); + } + private _collectSelectedChoiceIds(answer: any, selectedChoiceIds: Set): void { if (answer === null || answer === undefined) { return; diff --git a/projects/v3/src/app/components/oneof/oneof.component.html b/projects/v3/src/app/components/oneof/oneof.component.html index 53ba85301a..d56b65af60 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.html +++ b/projects/v3/src/app/components/oneof/oneof.component.html @@ -7,12 +7,23 @@

{{question.
- Learner's Answer - Reviewer's Answer + + Selected by reviewer + + Not selected by reviewer + + + + Learner's Answer + Reviewer's Answer + diff --git a/projects/v3/src/app/components/oneof/oneof.component.spec.ts b/projects/v3/src/app/components/oneof/oneof.component.spec.ts index 4d63ed46eb..87c32533fe 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.spec.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.spec.ts @@ -176,6 +176,115 @@ describe('OneofComponent', () => { }); }); + describe('when testing display-only preview mode', () => { + it('should show every reviewer-only choice with its reviewer selection state to the learner', () => { + component.question = { + reviewerOnly: true, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = { answer: 1 }; + component.review = { answer: 2 }; + + fixture.detectChanges(); + + const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); + const selectedChips = fixture.nativeElement.querySelectorAll('ion-chip.success'); + const notSelectedChips = fixture.nativeElement.querySelectorAll('ion-chip.orange'); + + expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); + expect(items.length).toBe(3); + expect(selectedChips.length).toBe(1); + expect(selectedChips[0].textContent.trim()).toBe('Selected by reviewer'); + expect(notSelectedChips.length).toBe(2); + expect(Array.from(notSelectedChips).every((chip: Element) => chip.textContent.trim() === 'Not selected by reviewer')).toBeTrue(); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); + }); + + it('should show every reviewer-only choice as not selected when the review answer is empty', () => { + component.question = { + reviewerOnly: true, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = {}; + component.review = {}; + + fixture.detectChanges(); + + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); + expect(fixture.nativeElement.querySelectorAll('ion-chip.success').length).toBe(0); + expect(fixture.nativeElement.querySelectorAll('ion-chip.orange').length).toBe(2); + }); + + it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { + component.question = { + reviewerOnly: true, + canAnswer: true, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = {}; + component.review = { answer: 2 }; + + fixture.detectChanges(); + + expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([2]); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); + expect(fixture.nativeElement.textContent).not.toContain('Not selected by reviewer'); + }); + + it('should preserve selected-only rendering for shared-audience questions', () => { + component.question = { + reviewerOnly: false, + canAnswer: false, + choices: [ + { id: 1, name: 'choice1' }, + { id: 2, name: 'choice2' }, + { id: 3, name: 'choice3' } + ], + audience: ['submitter', 'reviewer'] + }; + component.submissionStatus = 'feedback available'; + component.doAssessment = false; + component.doReview = false; + component.submission = { answer: 1 }; + component.review = { answer: 2 }; + + fixture.detectChanges(); + + expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); + expect(fixture.nativeElement.textContent).toContain("Learner's Answer"); + expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); + }); + }); + describe('triggerSave()', () => { beforeEach(() => { component.question = { id: 42, audience: [] }; diff --git a/projects/v3/src/app/components/oneof/oneof.component.ts b/projects/v3/src/app/components/oneof/oneof.component.ts index 132d577f99..87a7e727a4 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.ts @@ -210,8 +210,15 @@ export class OneofComponent implements AfterViewInit, ControlValueAccessor, OnIn return !this.doAssessment && !this.doReview && (this.submissionStatus === 'feedback available' || this.submissionStatus === 'pending review' || (this.submissionStatus === 'done' && this.reviewStatus === '')); } + get isReviewerOnlyLearnerFeedback(): boolean { + return this.isDisplayOnly + && this.question?.reviewerOnly === true + && this.question?.canAnswer === false + && this.submissionStatus === 'feedback available'; + } + get displayChoices(): Array { - if (!this.isDisplayOnly) { + if (!this.isDisplayOnly || this.isReviewerOnlyLearnerFeedback) { return this.question?.choices || []; } @@ -230,6 +237,10 @@ export class OneofComponent implements AfterViewInit, ControlValueAccessor, OnIn return (this.question?.choices || []).filter(choice => selectedIds.has(choice.id)); } + isReviewChoiceSelected(choiceId: string | number): boolean { + return this.review?.answer === choiceId; + } + // innerHTML text toggle onLabelToggle = (id: string): void => { this.onChange(id); From e1fad5a1d0149b0e35cb74c1234276de0937b728 Mon Sep 17 00:00:00 2001 From: trtshen Date: Tue, 21 Jul 2026 12:22:10 +0800 Subject: [PATCH 07/14] [CORE-8277] Enhance reviewer-only question feedback display --- docs/assessment-flow.md | 2 +- .../multiple/multiple.component.html | 29 +++++++++--- .../multiple/multiple.component.scss | 44 ++++++++++++++++++ .../multiple/multiple.component.spec.ts | 20 +++++---- .../app/components/oneof/oneof.component.html | 29 +++++++++--- .../app/components/oneof/oneof.component.scss | 45 +++++++++++++++++++ .../components/oneof/oneof.component.spec.ts | 20 +++++---- 7 files changed, 158 insertions(+), 31 deletions(-) diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index a3f48f2aef..dfe7709cdf 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -135,7 +135,7 @@ questionsForm: FormGroup = new FormGroup({}); 3. **Feedback Available**: Read-only with feedback - Display learner answers and reviewer feedback - - For reviewer-only `multiple` and `oneof` questions, display every configured choice and label it as either **Selected by reviewer** or **Not selected by reviewer** + - For reviewer-only `multiple` and `oneof` questions, display every configured choice with a green check and **Selected** status or a subdued **Not selected** status - Reviewer-only choice feedback is derived only from the review answer and does not display learner-answer labels - "Mark as Read" button to acknowledge feedback - Navigation to next task after reading diff --git a/projects/v3/src/app/components/multiple/multiple.component.html b/projects/v3/src/app/components/multiple/multiple.component.html index 0e713590d6..db84e3e8f9 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.html +++ b/projects/v3/src/app/components/multiple/multiple.component.html @@ -4,15 +4,30 @@

{ - -
+ +
- Selected by reviewer + + + + Selected by reviewer + - Not selected by reviewer + + + Not selected by reviewer + diff --git a/projects/v3/src/app/components/multiple/multiple.component.scss b/projects/v3/src/app/components/multiple/multiple.component.scss index 46fc455458..0f02e2c75f 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.scss +++ b/projects/v3/src/app/components/multiple/multiple.component.scss @@ -65,3 +65,47 @@ ion-item { --min-height: 1em; } +ion-label.reviewer-feedback-choice { + display: flex !important; + align-items: center; + justify-content: space-between; + gap: 16px; +} + +.reviewer-feedback-choice-text { + flex: 1 1 auto; + min-width: 0; +} + +.reviewer-choice-status { + display: inline-flex; + flex: 0 0 auto; + align-items: center; + gap: 4px; + font-size: 14px; + line-height: 20px; + white-space: nowrap; +} + +.reviewer-choice-status-selected { + color: var(--ion-color-success-shade); + font-weight: 700; + + ion-icon { + --ionicon-stroke-width: 48px; + font-size: 18px; + } +} + +.reviewer-choice-status-not-selected { + color: var(--practera-grey-50); + font-weight: 400; +} + +@media (max-width: 576px) { + ion-label.reviewer-feedback-choice { + align-items: flex-start; + flex-direction: column; + gap: 8px; + } +} diff --git a/projects/v3/src/app/components/multiple/multiple.component.spec.ts b/projects/v3/src/app/components/multiple/multiple.component.spec.ts index 5194ea25fa..36c01aa2f3 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.spec.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.spec.ts @@ -199,16 +199,20 @@ describe('MultipleComponent', () => { fixture.detectChanges(); const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); - const selectedChips = fixture.nativeElement.querySelectorAll('ion-chip.success'); - const notSelectedChips = fixture.nativeElement.querySelectorAll('ion-chip.orange'); + const selectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected'); + const notSelectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected'); expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); expect(items.length).toBe(3); - expect(selectedChips.length).toBe(1); - expect(selectedChips[0].textContent.trim()).toBe('Selected by reviewer'); - expect(notSelectedChips.length).toBe(2); - expect(Array.from(notSelectedChips).every((chip: Element) => chip.textContent.trim() === 'Not selected by reviewer')).toBeTrue(); + expect(selectedStatuses.length).toBe(1); + expect(selectedStatuses[0].querySelector('ion-icon').getAttribute('name')).toBe('checkmark'); + expect(selectedStatuses[0].querySelector('.reviewer-choice-status-text').textContent.trim()).toBe('Selected'); + expect(selectedStatuses[0].textContent).toContain('Selected by reviewer'); + expect(notSelectedStatuses.length).toBe(2); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.querySelector('ion-icon') === null)).toBeTrue(); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.querySelector('.reviewer-choice-status-text').textContent.trim() === 'Not selected')).toBeTrue(); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.textContent.includes('Not selected by reviewer'))).toBeTrue(); expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); }); @@ -232,8 +236,8 @@ describe('MultipleComponent', () => { fixture.detectChanges(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); - expect(fixture.nativeElement.querySelectorAll('ion-chip.success').length).toBe(0); - expect(fixture.nativeElement.querySelectorAll('ion-chip.orange').length).toBe(2); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected').length).toBe(0); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected').length).toBe(2); }); it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { diff --git a/projects/v3/src/app/components/oneof/oneof.component.html b/projects/v3/src/app/components/oneof/oneof.component.html index d56b65af60..3036dee480 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.html +++ b/projects/v3/src/app/components/oneof/oneof.component.html @@ -5,15 +5,30 @@

{{question. - -
+ +
- Selected by reviewer + + + + Selected by reviewer + - Not selected by reviewer + + + Not selected by reviewer + diff --git a/projects/v3/src/app/components/oneof/oneof.component.scss b/projects/v3/src/app/components/oneof/oneof.component.scss index c710298e38..2242e53ca5 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.scss +++ b/projects/v3/src/app/components/oneof/oneof.component.scss @@ -70,3 +70,48 @@ ion-item { .feedback-title { --min-height: 1em; } + +ion-label.reviewer-feedback-choice { + display: flex !important; + align-items: center; + justify-content: space-between; + gap: 16px; +} + +.reviewer-feedback-choice-text { + flex: 1 1 auto; + min-width: 0; +} + +.reviewer-choice-status { + display: inline-flex; + flex: 0 0 auto; + align-items: center; + gap: 4px; + font-size: 14px; + line-height: 20px; + white-space: nowrap; +} + +.reviewer-choice-status-selected { + color: var(--ion-color-success-shade); + font-weight: 700; + + ion-icon { + --ionicon-stroke-width: 48px; + font-size: 18px; + } +} + +.reviewer-choice-status-not-selected { + color: var(--practera-grey-50); + font-weight: 400; +} + +@media (max-width: 576px) { + ion-label.reviewer-feedback-choice { + align-items: flex-start; + flex-direction: column; + gap: 8px; + } +} diff --git a/projects/v3/src/app/components/oneof/oneof.component.spec.ts b/projects/v3/src/app/components/oneof/oneof.component.spec.ts index 87c32533fe..ed31ff3a76 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.spec.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.spec.ts @@ -197,16 +197,20 @@ describe('OneofComponent', () => { fixture.detectChanges(); const items = fixture.nativeElement.querySelectorAll('ion-list ion-item'); - const selectedChips = fixture.nativeElement.querySelectorAll('ion-chip.success'); - const notSelectedChips = fixture.nativeElement.querySelectorAll('ion-chip.orange'); + const selectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected'); + const notSelectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected'); expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); expect(items.length).toBe(3); - expect(selectedChips.length).toBe(1); - expect(selectedChips[0].textContent.trim()).toBe('Selected by reviewer'); - expect(notSelectedChips.length).toBe(2); - expect(Array.from(notSelectedChips).every((chip: Element) => chip.textContent.trim() === 'Not selected by reviewer')).toBeTrue(); + expect(selectedStatuses.length).toBe(1); + expect(selectedStatuses[0].querySelector('ion-icon').getAttribute('name')).toBe('checkmark'); + expect(selectedStatuses[0].querySelector('.reviewer-choice-status-text').textContent.trim()).toBe('Selected'); + expect(selectedStatuses[0].textContent).toContain('Selected by reviewer'); + expect(notSelectedStatuses.length).toBe(2); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.querySelector('ion-icon') === null)).toBeTrue(); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.querySelector('.reviewer-choice-status-text').textContent.trim() === 'Not selected')).toBeTrue(); + expect(Array.from(notSelectedStatuses).every((status: Element) => status.textContent.includes('Not selected by reviewer'))).toBeTrue(); expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); }); @@ -230,8 +234,8 @@ describe('OneofComponent', () => { fixture.detectChanges(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); - expect(fixture.nativeElement.querySelectorAll('ion-chip.success').length).toBe(0); - expect(fixture.nativeElement.querySelectorAll('ion-chip.orange').length).toBe(2); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected').length).toBe(0); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected').length).toBe(2); }); it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { From b59cce46b2cf24dfbd480da7312fabf5dba4b70e Mon Sep 17 00:00:00 2001 From: trtshen Date: Mon, 27 Jul 2026 09:30:29 +0800 Subject: [PATCH 08/14] [CORE-8277] reapplid new checkbox readonly for reviewer-view --- docs/assessment-flow.md | 1 + .../components/multiple/multiple.component.html | 6 +++--- .../multiple/multiple.component.spec.ts | 14 ++++++++------ .../components/multiple/multiple.component.ts | 5 ++--- .../app/components/oneof/oneof.component.html | 6 +++--- .../app/components/oneof/oneof.component.spec.ts | 16 +++++++++------- .../src/app/components/oneof/oneof.component.ts | 5 ++--- 7 files changed, 28 insertions(+), 25 deletions(-) diff --git a/docs/assessment-flow.md b/docs/assessment-flow.md index dfe7709cdf..dc353a11dd 100644 --- a/docs/assessment-flow.md +++ b/docs/assessment-flow.md @@ -149,6 +149,7 @@ questionsForm: FormGroup = new FormGroup({}); 2. **Review Complete**: Read-only mode - Show completed review + - For reviewer-only `multiple` and `oneof` questions, show every configured choice using the same **Selected** and **Not selected** statuses as the learner's published-feedback view - No further editing allowed #### Form Population Logic diff --git a/projects/v3/src/app/components/multiple/multiple.component.html b/projects/v3/src/app/components/multiple/multiple.component.html index db84e3e8f9..786fc0381d 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.html +++ b/projects/v3/src/app/components/multiple/multiple.component.html @@ -5,11 +5,11 @@

{ -
+
- + { const selectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected'); const notSelectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected'); - expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); + expect(component.isReviewerOnlyChoiceFeedback).toBeTrue(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); expect(items.length).toBe(3); expect(selectedStatuses.length).toBe(1); @@ -240,7 +240,7 @@ describe('MultipleComponent', () => { expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected').length).toBe(2); }); - it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { + it('should show every reviewer-only choice with its selection state in a completed reviewer view', () => { component.question = { reviewerOnly: true, canAnswer: true, @@ -259,10 +259,12 @@ describe('MultipleComponent', () => { fixture.detectChanges(); - expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); - expect(component.displayChoices.map(choice => choice.id)).toEqual([2]); - expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); - expect(fixture.nativeElement.textContent).not.toContain('Not selected by reviewer'); + expect(component.isReviewerOnlyChoiceFeedback).toBeTrue(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected').length).toBe(1); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected').length).toBe(2); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); }); }); diff --git a/projects/v3/src/app/components/multiple/multiple.component.ts b/projects/v3/src/app/components/multiple/multiple.component.ts index 5e1a037929..bfffcaa6f8 100644 --- a/projects/v3/src/app/components/multiple/multiple.component.ts +++ b/projects/v3/src/app/components/multiple/multiple.component.ts @@ -247,15 +247,14 @@ export class MultipleComponent implements AfterViewInit, ControlValueAccessor, O return !this.doAssessment && !this.doReview && (this.submissionStatus === 'feedback available' || this.submissionStatus === 'pending review' || (this.submissionStatus === 'done' && this.reviewStatus === '')); } - get isReviewerOnlyLearnerFeedback(): boolean { + get isReviewerOnlyChoiceFeedback(): boolean { return this.isDisplayOnly && this.question?.reviewerOnly === true - && this.question?.canAnswer === false && this.submissionStatus === 'feedback available'; } get displayChoices(): Array { - if (!this.isDisplayOnly || this.isReviewerOnlyLearnerFeedback) { + if (!this.isDisplayOnly || this.isReviewerOnlyChoiceFeedback) { return this.question?.choices || []; } diff --git a/projects/v3/src/app/components/oneof/oneof.component.html b/projects/v3/src/app/components/oneof/oneof.component.html index 3036dee480..f3600ad41f 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.html +++ b/projects/v3/src/app/components/oneof/oneof.component.html @@ -6,11 +6,11 @@

{{question. -
+
- + { const selectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected'); const notSelectedStatuses = fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected'); - expect(component.isReviewerOnlyLearnerFeedback).toBeTrue(); + expect(component.isReviewerOnlyChoiceFeedback).toBeTrue(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); expect(items.length).toBe(3); expect(selectedStatuses.length).toBe(1); @@ -238,7 +238,7 @@ describe('OneofComponent', () => { expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected').length).toBe(2); }); - it('should preserve selected-only rendering when a reviewer reopens a completed review', () => { + it('should show every reviewer-only choice with its selection state in a completed reviewer view', () => { component.question = { reviewerOnly: true, canAnswer: true, @@ -257,10 +257,12 @@ describe('OneofComponent', () => { fixture.detectChanges(); - expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); - expect(component.displayChoices.map(choice => choice.id)).toEqual([2]); - expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); - expect(fixture.nativeElement.textContent).not.toContain('Not selected by reviewer'); + expect(component.isReviewerOnlyChoiceFeedback).toBeTrue(); + expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2, 3]); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-selected').length).toBe(1); + expect(fixture.nativeElement.querySelectorAll('.reviewer-choice-status-not-selected').length).toBe(2); + expect(fixture.nativeElement.textContent).not.toContain("Learner's Answer"); + expect(fixture.nativeElement.textContent).not.toContain("Reviewer's Answer"); }); it('should preserve selected-only rendering for shared-audience questions', () => { @@ -282,7 +284,7 @@ describe('OneofComponent', () => { fixture.detectChanges(); - expect(component.isReviewerOnlyLearnerFeedback).toBeFalse(); + expect(component.isReviewerOnlyChoiceFeedback).toBeFalse(); expect(component.displayChoices.map(choice => choice.id)).toEqual([1, 2]); expect(fixture.nativeElement.textContent).toContain("Learner's Answer"); expect(fixture.nativeElement.textContent).toContain("Reviewer's Answer"); diff --git a/projects/v3/src/app/components/oneof/oneof.component.ts b/projects/v3/src/app/components/oneof/oneof.component.ts index 87a7e727a4..a0e4f66567 100644 --- a/projects/v3/src/app/components/oneof/oneof.component.ts +++ b/projects/v3/src/app/components/oneof/oneof.component.ts @@ -210,15 +210,14 @@ export class OneofComponent implements AfterViewInit, ControlValueAccessor, OnIn return !this.doAssessment && !this.doReview && (this.submissionStatus === 'feedback available' || this.submissionStatus === 'pending review' || (this.submissionStatus === 'done' && this.reviewStatus === '')); } - get isReviewerOnlyLearnerFeedback(): boolean { + get isReviewerOnlyChoiceFeedback(): boolean { return this.isDisplayOnly && this.question?.reviewerOnly === true - && this.question?.canAnswer === false && this.submissionStatus === 'feedback available'; } get displayChoices(): Array { - if (!this.isDisplayOnly || this.isReviewerOnlyLearnerFeedback) { + if (!this.isDisplayOnly || this.isReviewerOnlyChoiceFeedback) { return this.question?.choices || []; } From 1ac119c582bd82d1f797066e656180c06dae1348 Mon Sep 17 00:00:00 2001 From: trtshen Date: Thu, 6 Aug 2026 14:30:26 +0800 Subject: [PATCH 09/14] [CORE-8309] 2.4.y/upload-profile-pic --- docs/docs.md | 3 +- docs/fixes/profile-picture-upload.md | 56 +++++++++++++ .../file-upload/file-upload.component.spec.ts | 7 +- .../file-upload/file-upload.component.ts | 18 ++--- .../uppy-uploader.component.spec.ts | 78 +++++++++++++++++++ .../uppy-uploader/uppy-uploader.component.ts | 34 +++++--- .../uppy-uploader.service.spec.ts | 39 ++++++++++ .../uppy-uploader/uppy-uploader.service.ts | 44 ++++++++++- .../app/pages/settings/settings.page.spec.ts | 61 ++++++++++++++- .../src/app/pages/settings/settings.page.ts | 28 ++++--- .../v3/src/app/services/auth.service.spec.ts | 22 ++++++ projects/v3/src/app/services/auth.service.ts | 14 ++-- 12 files changed, 361 insertions(+), 43 deletions(-) create mode 100644 docs/fixes/profile-picture-upload.md create mode 100644 projects/v3/src/app/components/uppy-uploader/uppy-uploader.component.spec.ts diff --git a/docs/docs.md b/docs/docs.md index 3e72cd1b3a..c0566222ae 100644 --- a/docs/docs.md +++ b/docs/docs.md @@ -34,4 +34,5 @@ This is practera documentation with more informations. ### Fixes - [CORE-7942 Whitespace Fix](./fixes/CORE-7942-whitespace-fix.md) - [CORE-8002 Pulse Check Workflow](./fixes/CORE-8002-pulsecheck-workflow.md) -- [CORE-8166/8167 Pagination Answer Persistence](./fixes/CORE-8166-8167-pagination-answer-persistence.md) \ No newline at end of file +- [CORE-8166/8167 Pagination Answer Persistence](./fixes/CORE-8166-8167-pagination-answer-persistence.md) +- [Profile Picture Upload](./fixes/profile-picture-upload.md) diff --git a/docs/fixes/profile-picture-upload.md b/docs/fixes/profile-picture-upload.md new file mode 100644 index 0000000000..7a12657e89 --- /dev/null +++ b/docs/fixes/profile-picture-upload.md @@ -0,0 +1,56 @@ +--- +status: stable +authority: reference +scope: v3 +last_reviewed: 2026-08-06 +supersedes: none +--- + +# Profile Picture Upload + +## Failure + +The settings page uploaded the image successfully but could send an invalid `FileInput` to the `updateUserProfile` mutation. The upload modal read the TUS response as if it contained `url`, while the upload service returns `cdnUrl`. It therefore dismissed the modal with an undefined `url`, and the settings page replaced that value with the resumable TUS `uploadUrl`. That URL is an upload-session location, not the public image URL expected by the profile API. + +The settings page also ignored the mutation result. A response such as `{ success: false, message: "avatar file object incorrect" }` still updated local state and displayed the success alert. + +`AuthService.updateUserProfile()` also passed the mutation document to `graphQLFetch()`, which executes `Apollo.query()`. Apollo rejected the request locally with `Running a query requires a graphql query, but a mutation was used instead`, before it could reach the profile resolver. Profile updates must use `graphQLMutate()` with `{ avatar }` as the variables object. + +## Upload response contract + +The final TUS `PATCH` response must have a JSON body containing non-empty values for: + +```json +{ + "bucket": "profile-images", + "path": "/users/profile.png", + "cdnUrl": "https://cdn.example.com/users/profile.png", + "directUrl": "https://files.example.com/users/profile.png" +} +``` + +`UppyUploaderService.parseTusUploadResponse()` is the shared validator used by the modal uploader and assessment file uploader. Empty, malformed, or incomplete response bodies stop the upload flow with a specific user-visible error. + +The modal normalizes the response to `UppyFileData` and preserves both `cdnUrl` and `directUrl`. Assessment answers persist the canonical CDN URL but prefer `directUrl || url` for immediate display. Profile avatars follow the same display preference and persist `directUrl` when it is available because the `user-profile` CDN URL may not be directly readable; they fall back to the canonical `url`. Consumers must never use `file.tus.uploadUrl` as stored file metadata. + +## Profile update behavior + +`SettingsPage.profileImage()` sends the normalized file fields to `AuthService.updateUserProfile()`: + +- `bucket` +- `path` +- `name` +- `url` (`directUrl` when available, otherwise the CDN URL) +- `extension` +- `type` +- `size` + +The page updates its avatar and browser storage only when `data.updateUserProfile.success` is exactly `true`. A missing result or `success: false` displays the returned message and leaves the previous avatar unchanged. The upload spinner is cleared for success, cancellation, and error paths. + +The `user-profile` upload source is image-only. + +## Verification and rollout + +Automated coverage verifies TUS response validation, assessment uploader integration, image-only profile restrictions, direct-URL preference, successful profile payloads, and rejected mutations. + +After deployment, verify one successful PNG/JPEG upload and one rejected/invalid upload in staging. Monitor upload endpoint errors and `updateUserProfile` failures separately. Logs should include the request/correlation identifier, upload source, HTTP status, and a stable error category such as `empty_upload_response`, `invalid_upload_metadata`, or `profile_update_rejected`; they must not include file bytes, API keys, or full signed URLs. diff --git a/projects/v3/src/app/components/file-upload/file-upload.component.spec.ts b/projects/v3/src/app/components/file-upload/file-upload.component.spec.ts index 165263004c..f7e24c5b3d 100644 --- a/projects/v3/src/app/components/file-upload/file-upload.component.spec.ts +++ b/projects/v3/src/app/components/file-upload/file-upload.component.spec.ts @@ -9,7 +9,11 @@ describe('FileUploadComponent', () => { let uppyUploaderService: jasmine.SpyObj; beforeEach(() => { - uppyUploaderService = jasmine.createSpyObj('UppyUploaderService', ['createUppyInstance']); + uppyUploaderService = jasmine.createSpyObj('UppyUploaderService', [ + 'createUppyInstance', + 'parseTusUploadResponse', + ]); + uppyUploaderService.parseTusUploadResponse.and.callFake((body) => JSON.parse(body)); component = new FileUploadComponent(uppyUploaderService); component.control = new FormControl(''); component.submitActions$ = new Subject(); @@ -59,6 +63,7 @@ describe('FileUploadComponent', () => { component.onAfterResponse({}, response); + expect(uppyUploaderService.parseTusUploadResponse).toHaveBeenCalledWith(response.getBody()); expect(component.tusResponse).toEqual({ path: '/uploads/a', bucket: 'b', cdnUrl: 'c', directUrl: 'd' }); }); diff --git a/projects/v3/src/app/components/file-upload/file-upload.component.ts b/projects/v3/src/app/components/file-upload/file-upload.component.ts index 0298e4ec06..cb7f7b230b 100644 --- a/projects/v3/src/app/components/file-upload/file-upload.component.ts +++ b/projects/v3/src/app/components/file-upload/file-upload.component.ts @@ -1,4 +1,9 @@ -import { UppyUploaderService, ALLOWED_FILE_TYPES } from './../uppy-uploader/uppy-uploader.service'; +import { + UppyUploaderService, + ALLOWED_FILE_TYPES, + TusUploadResponse, + UppyUploadSource, +} from './../uppy-uploader/uppy-uploader.service'; import { Component, ElementRef, Input, OnDestroy, OnInit, ViewChild, ViewEncapsulation } from '@angular/core'; import { AbstractControl } from '@angular/forms'; import { Subject } from 'rxjs'; @@ -41,7 +46,7 @@ export class FileUploadComponent implements OnInit, OnDestroy { // Uppy UI uppyProps = UPPY_PROPS; - @Input() source!: "chat" | "profile" | "assessment" | "any" | "video" | "document" | "image"; + @Input() source!: UppyUploadSource; @Input() submitActions$: Subject; @Input() videoOnly?: boolean; @@ -77,12 +82,7 @@ export class FileUploadComponent implements OnInit, OnDestroy { uploadedFile: TusFileResponse; fileTypes = ''; - tusResponse: { - path: string; - bucket: string; - cdnUrl: string; - directUrl: string; - }; + tusResponse: TusUploadResponse; // the value of answer innerValue: any; @@ -139,7 +139,7 @@ export class FileUploadComponent implements OnInit, OnDestroy { onAfterResponse(req: any, res: any): void { // eslint-disable-next-line no-console console.log('onAfterResponse', req, res); - this.tusResponse = JSON.parse(res.getBody()); + this.tusResponse = this.uppyUploaderService.parseTusUploadResponse(res.getBody()); } initializeEventHandlers(uppy) { diff --git a/projects/v3/src/app/components/uppy-uploader/uppy-uploader.component.spec.ts b/projects/v3/src/app/components/uppy-uploader/uppy-uploader.component.spec.ts new file mode 100644 index 0000000000..fddb32b16e --- /dev/null +++ b/projects/v3/src/app/components/uppy-uploader/uppy-uploader.component.spec.ts @@ -0,0 +1,78 @@ +import { ModalController } from '@ionic/angular'; +import { NotificationsService } from '../../services/notifications.service'; +import { BrowserStorageService } from '../../services/storage.service'; +import { UppyUploaderComponent } from './uppy-uploader.component'; +import { TusUploadResponse, UppyUploaderService } from './uppy-uploader.service'; + +describe('UppyUploaderComponent', () => { + let component: UppyUploaderComponent; + let notificationsService: jasmine.SpyObj; + let modalController: jasmine.SpyObj; + let storageService: jasmine.SpyObj; + let uppyUploaderService: jasmine.SpyObj; + + beforeEach(() => { + notificationsService = jasmine.createSpyObj('NotificationsService', ['alert']); + modalController = jasmine.createSpyObj('ModalController', ['dismiss']); + storageService = jasmine.createSpyObj('BrowserStorageService', ['clearByName']); + uppyUploaderService = jasmine.createSpyObj( + 'UppyUploaderService', + ['createUppyInstance', 'parseTusUploadResponse'], + { uppyProps: {} as any } + ); + + component = new UppyUploaderComponent( + notificationsService, + modalController, + storageService, + uppyUploaderService + ); + }); + + it('restricts user profile uploads to images', () => { + component.source = 'user-profile'; + + expect(component.loadAllowedFileTypes()).toEqual(['image/*']); + }); + + it('returns the canonical CDN URL from the TUS response', () => { + const tusResponse: TusUploadResponse = { + bucket: 'profile-images', + path: '/users/profile.png', + cdnUrl: 'https://cdn.example.com/users/profile.png', + directUrl: 'https://files.example.com/users/profile.png', + }; + component.s3Info = tusResponse; + const file = { + name: 'profile.png', + type: 'image/png', + size: 10, + extension: 'png', + } as any; + + component.closeModal(file); + + expect(modalController.dismiss).toHaveBeenCalledWith(jasmine.objectContaining({ + bucket: tusResponse.bucket, + path: tusResponse.path, + url: tusResponse.cdnUrl, + cdnUrl: tusResponse.cdnUrl, + directUrl: tusResponse.directUrl, + })); + }); + + it('reports and rethrows an invalid TUS response', () => { + uppyUploaderService.parseTusUploadResponse.and.throwError( + 'Upload server returned an empty response.' + ); + const response = { getBody: () => '' }; + + expect(() => component.onAfterResponse({}, response)).toThrowError( + 'Upload server returned an empty response.' + ); + expect(notificationsService.alert).toHaveBeenCalledWith({ + header: 'Upload Failed', + message: 'Upload server returned an empty response.', + }); + }); +}); diff --git a/projects/v3/src/app/components/uppy-uploader/uppy-uploader.component.ts b/projects/v3/src/app/components/uppy-uploader/uppy-uploader.component.ts index 3be19ae403..388baa4bef 100644 --- a/projects/v3/src/app/components/uppy-uploader/uppy-uploader.component.ts +++ b/projects/v3/src/app/components/uppy-uploader/uppy-uploader.component.ts @@ -1,4 +1,10 @@ -import { UppyFileData, UppyUploaderService, ALLOWED_FILE_TYPES } from './uppy-uploader.service'; +import { + UppyFileData, + UppyUploaderService, + ALLOWED_FILE_TYPES, + TusUploadResponse, + UppyUploadSource, +} from './uppy-uploader.service'; import { environment } from '@v3/environments/environment'; import { NotificationsService } from './../../services/notifications.service'; import { Component, OnInit, Input, Output, EventEmitter, OnDestroy } from '@angular/core'; @@ -16,7 +22,7 @@ type FileBody = { [key: string]: any }; styleUrls: ["./uppy-uploader.component.scss"], }) export class UppyUploaderComponent implements OnInit, OnDestroy { - @Input() source!: "chat" | "profile" | "assessment" | "any" | "video" | "document" | "image"; + @Input() source!: UppyUploadSource; @Input() tusEndpoint?: string = environment.uppyConfig.tusUrl; // tusUrl @Output() uploadComplete = new EventEmitter(); @@ -26,11 +32,7 @@ export class UppyUploaderComponent implements OnInit, OnDestroy { // Uppy UI uppyProps: any; - s3Info: { - path: string; - bucket: string; - url: string; - }; + s3Info: TusUploadResponse; constructor( private notificationsService: NotificationsService, @@ -67,6 +69,7 @@ export class UppyUploaderComponent implements OnInit, OnDestroy { loadAllowedFileTypes() { switch(this.source) { case "profile": + case "user-profile": case "image": return ["image/*"]; @@ -92,12 +95,13 @@ export class UppyUploaderComponent implements OnInit, OnDestroy { try { // eslint-disable-next-line no-console console.log("Uploaded files:", req, res); - this.s3Info = JSON.parse(res.getBody()); + this.s3Info = this.uppyUploaderService.parseTusUploadResponse(res.getBody()); } catch(error) { this.notificationsService.alert({ header: "Upload Failed", - message: "No response from server", + message: error.message, }); + throw error; } } @@ -113,12 +117,18 @@ export class UppyUploaderComponent implements OnInit, OnDestroy { } closeModal(file) { + if (!this.s3Info) { + throw new Error('Upload server response is missing required file metadata.'); + } + const data: UppyFileData = { ...file, ...{ - bucket: this.s3Info?.bucket, - path: this.s3Info?.path, - url: this.s3Info?.url, + bucket: this.s3Info.bucket, + path: this.s3Info.path, + url: this.s3Info.cdnUrl, + cdnUrl: this.s3Info.cdnUrl, + directUrl: this.s3Info.directUrl, } }; this.modalController.dismiss(data); diff --git a/projects/v3/src/app/components/uppy-uploader/uppy-uploader.service.spec.ts b/projects/v3/src/app/components/uppy-uploader/uppy-uploader.service.spec.ts index 4c43d8f7a4..73da77e0c5 100644 --- a/projects/v3/src/app/components/uppy-uploader/uppy-uploader.service.spec.ts +++ b/projects/v3/src/app/components/uppy-uploader/uppy-uploader.service.spec.ts @@ -142,4 +142,43 @@ describe('UppyUploaderService', () => { expect(service.getPatchValue(testId)).toEqual(testValue); }); }); + + describe('parseTusUploadResponse', () => { + it('should parse the upload metadata returned by the TUS server', () => { + const response = service.parseTusUploadResponse(JSON.stringify({ + bucket: 'bucket', + path: '/uploads/profile.png', + cdnUrl: 'https://cdn.example.com/profile.png', + directUrl: 'https://files.example.com/profile.png', + })); + + expect(response).toEqual({ + bucket: 'bucket', + path: '/uploads/profile.png', + cdnUrl: 'https://cdn.example.com/profile.png', + directUrl: 'https://files.example.com/profile.png', + }); + }); + + it('should reject an empty response body', () => { + expect(() => service.parseTusUploadResponse('')).toThrowError( + 'Upload server returned an empty response.' + ); + }); + + it('should reject malformed JSON', () => { + expect(() => service.parseTusUploadResponse('{invalid')).toThrowError( + 'Upload server returned an invalid response.' + ); + }); + + it('should reject incomplete upload metadata', () => { + expect(() => service.parseTusUploadResponse(JSON.stringify({ + bucket: 'bucket', + path: '/uploads/profile.png', + }))).toThrowError( + 'Upload server response is missing required file metadata.' + ); + }); + }); }); diff --git a/projects/v3/src/app/components/uppy-uploader/uppy-uploader.service.ts b/projects/v3/src/app/components/uppy-uploader/uppy-uploader.service.ts index 9407a7f6e2..8d0c20644a 100644 --- a/projects/v3/src/app/components/uppy-uploader/uppy-uploader.service.ts +++ b/projects/v3/src/app/components/uppy-uploader/uppy-uploader.service.ts @@ -17,6 +17,25 @@ export interface UppyUploaderResponse { size: number; } +export type UppyUploadSource = + | 'chat' + | 'profile' + | 'user-profile' + | 'assessment' + | 'media-manager' + | 'static' + | 'any' + | 'video' + | 'document' + | 'image'; + +export interface TusUploadResponse { + path: string; + bucket: string; + cdnUrl: string; + directUrl: string; +} + export interface UppyFileData { source: string; id: string; @@ -48,6 +67,8 @@ export interface UppyFileData { bucket: string; path: string; url: string; + cdnUrl: string; + directUrl: string; } type FileMetadata = { [key: string]: any }; @@ -113,7 +134,7 @@ export class UppyUploaderService { * @param restrictions * @returns Uppy */ - createUppyInstance(source: "chat" | "profile" | "assessment" | "any" | "video" | "document" | "image", uploadUrl: string, events?: { + createUppyInstance(source: UppyUploadSource, uploadUrl: string, events?: { onAfterResponse: (req: any, res: any) => void, onUploadSuccess: (file: UppyFile, response: any) => void }, options?: { @@ -165,6 +186,25 @@ export class UppyUploaderService { return uppy; } + parseTusUploadResponse(body: string): TusUploadResponse { + if (!body?.trim()) { + throw new Error('Upload server returned an empty response.'); + } + + let response: Partial; + try { + response = JSON.parse(body); + } catch { + throw new Error('Upload server returned an invalid response.'); + } + + if (!response.bucket || !response.path || !response.cdnUrl || !response.directUrl) { + throw new Error('Upload server response is missing required file metadata.'); + } + + return response as TusUploadResponse; + } + private initializeEventHandlers(uppy: Uppy, onUploadSuccess: (file: UppyFile, response: any) => void) { uppy.on('dashboard:file-edit-start', (file: any) => { console.log('file edit start', file); @@ -198,7 +238,7 @@ export class UppyUploaderService { * @param {string} source * @return {Promise} */ - async open(source: 'chat' | 'user-profile' | 'assessment' | 'media-manager' | 'static' | 'any' | 'image' | 'video' | null): Promise { + async open(source: UppyUploadSource | null): Promise { // dynamic import to break circular dependency with UppyUploaderComponent const { UppyUploaderComponent } = await import('./uppy-uploader.component'); const modal = await this.modalController.create({ diff --git a/projects/v3/src/app/pages/settings/settings.page.spec.ts b/projects/v3/src/app/pages/settings/settings.page.spec.ts index f79fe0b1c7..e0fbea0622 100644 --- a/projects/v3/src/app/pages/settings/settings.page.spec.ts +++ b/projects/v3/src/app/pages/settings/settings.page.spec.ts @@ -57,7 +57,14 @@ describe('SettingsPage', () => { } } as any)); authSpy.logout.and.returnValue(Promise.resolve() as any); - authSpy.updateUserProfile.and.returnValue(of({}) as any); + authSpy.updateUserProfile.and.returnValue(of({ + data: { + updateUserProfile: { + success: true, + message: 'User profile updated successfully', + } + } + }) as any); storageSpy.getUser.and.returnValue({ email: 'user@example.com', @@ -235,6 +242,8 @@ describe('SettingsPage', () => { size: 10, bucket: 'bucket', path: '/uploads/profile', + url: 'https://cdn/profile.png', + directUrl: 'https://files/profile.png', preview: 'https://cdn/profile.png', }; uppyUploaderServiceSpy.open.and.returnValue(Promise.resolve({ @@ -243,12 +252,56 @@ describe('SettingsPage', () => { await component.profileImage(); - expect(authSpy.updateUserProfile).toHaveBeenCalled(); - expect(component.profile.avatar).toBe('https://cdn/profile.png'); - expect(storageSpy.setUser).toHaveBeenCalledWith({ image: 'https://cdn/profile.png' }); + expect(authSpy.updateUserProfile).toHaveBeenCalledWith({ + url: 'https://files/profile.png', + name: 'profile.png', + extension: 'png', + type: 'image/png', + size: 10, + bucket: 'bucket', + path: '/uploads/profile', + }); + expect(component.profile.avatar).toBe('https://files/profile.png'); + expect(storageSpy.setUser).toHaveBeenCalledWith({ + avatar: 'https://files/profile.png', + image: 'https://files/profile.png', + }); expect(notificationsServiceSpy.alert).toHaveBeenCalled(); }); + it('should not update local profile when the backend rejects the file', async () => { + const uploaded = { + tus: { uploadUrl: 'https://upload' }, + name: 'profile.png', + extension: 'png', + type: 'image/png', + size: 10, + bucket: 'bucket', + path: '/uploads/profile', + url: 'https://cdn/profile.png', + directUrl: 'https://files/profile.png', + }; + uppyUploaderServiceSpy.open.and.returnValue(Promise.resolve({ + onDidDismiss: () => Promise.resolve({ data: uploaded }) + } as any)); + authSpy.updateUserProfile.and.returnValue(of({ + data: { + updateUserProfile: { + success: false, + message: 'avatar file object incorrect', + } + } + }) as any); + + await component.profileImage(); + + expect(component.profile.avatar).not.toBe('https://files/profile.png'); + expect(storageSpy.setUser).not.toHaveBeenCalled(); + const alertArgs = notificationsServiceSpy.alert.calls.mostRecent().args[0]; + expect(alertArgs.subHeader).toBe('avatar file object incorrect'); + expect(component.imageUpdating).toBeFalse(); + }); + it('should show upload error subHeader when server returns message', async () => { uppyUploaderServiceSpy.open.and.returnValue(Promise.resolve({ onDidDismiss: () => Promise.resolve({ data: { tus: { uploadUrl: 'u' } } }) diff --git a/projects/v3/src/app/pages/settings/settings.page.ts b/projects/v3/src/app/pages/settings/settings.page.ts index fff0999a3f..eb89367fc5 100644 --- a/projects/v3/src/app/pages/settings/settings.page.ts +++ b/projects/v3/src/app/pages/settings/settings.page.ts @@ -186,8 +186,11 @@ export class SettingsPage implements OnInit, OnDestroy { const file = res.data; if (file) { this.imageUpdating = true; - await firstValueFrom(this.authService.updateUserProfile({ - url: file.tus.uploadUrl, + // User-profile CDN URLs are not directly readable in every environment. + // Match file-display and prefer the TUS direct URL when it is available. + const profileUrl = file.directUrl || file.url; + const response = await firstValueFrom(this.authService.updateUserProfile({ + url: profileUrl, name: file.name, extension: file.extension, type: file.type, @@ -196,9 +199,16 @@ export class SettingsPage implements OnInit, OnDestroy { path: file.path, })); - this.imageUpdating = false; - this.profile.avatar = file.preview; - this.storage.setUser({ image: file.preview }); + const result = response?.data?.updateUserProfile; + if (result?.success !== true) { + throw new Error(result?.message || 'Profile picture could not be updated.'); + } + + this.profile.avatar = profileUrl; + this.storage.setUser({ + avatar: profileUrl, + image: profileUrl, + }); return this.notificationsService.alert({ message: $localize`Profile picture successfully updated!`, @@ -211,8 +221,6 @@ export class SettingsPage implements OnInit, OnDestroy { }); } } catch (error) { - this.imageUpdating = false; - // eslint-disable-next-line no-console console.error('profile image error', error); @@ -227,10 +235,12 @@ export class SettingsPage implements OnInit, OnDestroy { }; // Actual error message from server - if (error?.error?.message || error?.error?.msg) { - alertOpts.subHeader = error?.error?.message || error?.error?.msg; + if (error?.error?.message || error?.error?.msg || error?.message) { + alertOpts.subHeader = error?.error?.message || error?.error?.msg || error?.message; } return this.notificationsService.alert(alertOpts); + } finally { + this.imageUpdating = false; } } diff --git a/projects/v3/src/app/services/auth.service.spec.ts b/projects/v3/src/app/services/auth.service.spec.ts index a29932a99b..9a089b85ff 100644 --- a/projects/v3/src/app/services/auth.service.spec.ts +++ b/projects/v3/src/app/services/auth.service.spec.ts @@ -45,6 +45,7 @@ describe('AuthService', () => { provide: ApolloService, useValue: jasmine.createSpyObj('ApolloService', { 'graphQLFetch': of(), + 'graphQLMutate': of(), 'graphQLWatch': of(), 'getClient': function () { return { @@ -103,6 +104,27 @@ describe('AuthService', () => { expect(service).toBeTruthy(); }); + it('should execute updateUserProfile as a mutation with the avatar variables', () => { + const apolloSpy = TestBed.inject(ApolloService) as jasmine.SpyObj; + const avatar = { + bucket: 'profile-images', + path: '/users/profile.png', + name: 'profile.png', + url: 'https://cdn.example.com/users/profile.png', + extension: 'png', + type: 'image/png', + size: 10, + }; + + service.updateUserProfile(avatar).subscribe(); + + expect(apolloSpy.graphQLMutate).toHaveBeenCalledWith( + jasmine.stringMatching(/mutation updateUserProfile/), + { avatar } + ); + expect(apolloSpy.graphQLFetch).not.toHaveBeenCalled(); + }); + it('when testing directLogin(), it should pass the correct data to API', () => { const apolloSpy = TestBed.inject(ApolloService) as jasmine.SpyObj; apolloSpy.graphQLFetch.and.returnValue(of({ diff --git a/projects/v3/src/app/services/auth.service.ts b/projects/v3/src/app/services/auth.service.ts index 5a132ca8fb..cb871222a4 100644 --- a/projects/v3/src/app/services/auth.service.ts +++ b/projects/v3/src/app/services/auth.service.ts @@ -45,6 +45,12 @@ interface ProfileAvatar { size: number; } +interface UpdateUserProfileResponse { + data?: { + updateUserProfile?: Response; + }; +} + interface RegisterData { password?: string; user_id: number; @@ -669,8 +675,8 @@ export class AuthService { * * @return {} [return description] */ - updateUserProfile(avatar: ProfileAvatar): Observable { - return this.apolloService.graphQLFetch(` + updateUserProfile(avatar: ProfileAvatar): Observable { + return this.apolloService.graphQLMutate(` mutation updateUserProfile($avatar: FileInput) { updateUserProfile(avatar: $avatar) { success @@ -678,9 +684,7 @@ export class AuthService { } } `, { - variables: { - avatar - } + avatar }); } } From f31e4414d085281b2425b1b0e6377712bc478536 Mon Sep 17 00:00:00 2001 From: trtshen Date: Fri, 7 Aug 2026 11:05:21 +0800 Subject: [PATCH 10/14] [CORE-6152] 2.4.y/accessWidget-blockage --- docs/accessibility/WCAG_CHECKLIST.md | 11 +++- .../personalised-header.component.html | 11 ++++ .../personalised-header.component.scss | 3 ++ .../personalised-header.component.spec.ts | 13 +++++ projects/v3/src/index.html | 53 ++++++++++++++++++- projects/v3/src/styles.scss | 17 ++++++ 6 files changed, 105 insertions(+), 3 deletions(-) diff --git a/docs/accessibility/WCAG_CHECKLIST.md b/docs/accessibility/WCAG_CHECKLIST.md index 43f84ac1ea..cae2af1849 100644 --- a/docs/accessibility/WCAG_CHECKLIST.md +++ b/docs/accessibility/WCAG_CHECKLIST.md @@ -123,6 +123,9 @@ This checklist verifies compliance with WCAG 2.2 Level AA standards for the V3 I - [x] **COMPLETED**: Added CSS to prevent focus obscuring (scroll-margin: 4px on focus-visible elements) - [x] Sticky headers/footers have proper z-index (ion-header and ion-footer set to z-index: 1000) - [x] Modals/overlays configured with backdrop opacity (ion-modal has --backdrop-opacity: 0.4) +- [x] **COMPLETED**: Replaced accessWidget's production floating trigger with a keyboard-accessible custom trigger in the personalised header +- [x] **COMPLETED**: Reserved production-only inline space in the chat action row so a vendor-overridden floating trigger cannot obscure Attach or Send +- [ ] **RETEST ON DEPLOYED HOST**: Verify the custom trigger opens accessWidget and the chat controls remain unobscured at desktop, tablet, and mobile viewports #### 2.4.13 Focus Appearance (Minimum) (Level AA) - NEW in 2.2 - [x] Focus indicators have at least 2px outline (implemented in global.scss) @@ -394,7 +397,7 @@ This checklist verifies compliance with WCAG 2.2 Level AA standards for the V3 I 7. Test with keyboard: Tab to element, tooltip should appear; ESC should dismiss it #### 5. Focus Not Obscured (WCAG 2.4.11) -**Fixed in:** `global.scss` +**Fixed in:** `global.scss`, `styles.scss`, `personalised-header.component.html`, and `index.html` **Retest Instructions:** 1. Navigate to any page with sticky header/footer @@ -404,6 +407,11 @@ This checklist verifies compliance with WCAG 2.2 Level AA standards for the V3 I 5. Open DevTools and check computed styles on focused element: - `scroll-margin: 4px` should be present - `z-index` on headers/footers should be 1000 +6. On a deployed non-local host, Tab to the header button labelled "Open accessibility options" +7. **VERIFY**: Enter and Space open accessWidget and the button remains visible with support, notification, and profile controls +8. Navigate to Messages and select a writable chat room +9. **VERIFY**: Attach and Send remain visible and clickable at 2048x1048, 1366x768, 1024x768, and 390x844 +10. **VERIFY**: If accessiBe overrides `hideTrigger`, the remaining floating trigger does not obscure the chat action row #### 6. Text Spacing Support (WCAG 1.4.12) **Fixed in:** `global.scss` @@ -494,4 +502,3 @@ This checklist verifies compliance with WCAG 2.2 Level AA standards for the V3 I - Assessment component has loading states with proper ARIA - Error messages use role="alert" and aria-live="assertive" - Status messages use role="status" and aria-live="polite" - diff --git a/projects/v3/src/app/personalised-header/personalised-header.component.html b/projects/v3/src/app/personalised-header/personalised-header.component.html index 799de5f020..3db5ace695 100644 --- a/projects/v3/src/app/personalised-header/personalised-header.component.html +++ b/projects/v3/src/app/personalised-header/personalised-header.component.html @@ -25,6 +25,17 @@ + + + + { it('should create', () => { expect(component).toBeTruthy(); }); + + it('should expose the accessWidget custom trigger before notifications', () => { + const accessibilityButton: HTMLElement = fixture.nativeElement.querySelector('.accessibility-btn'); + const notificationButton: HTMLElement = fixture.nativeElement.querySelector('.notify-btn'); + const icon: HTMLElement = accessibilityButton.querySelector('ion-icon'); + + expect(accessibilityButton).toBeTruthy(); + expect(accessibilityButton.getAttribute('aria-label')).toBe('Open accessibility options'); + expect(accessibilityButton.getAttribute('data-acsb-custom-trigger')).toBe('true'); + expect(icon.getAttribute('name')).toBe('accessibility-outline'); + expect(accessibilityButton.compareDocumentPosition(notificationButton) & Node.DOCUMENT_POSITION_FOLLOWING) + .toBeTruthy(); + }); }); diff --git a/projects/v3/src/index.html b/projects/v3/src/index.html index 9cc5fe6018..8b10cc5f88 100644 --- a/projects/v3/src/index.html +++ b/projects/v3/src/index.html @@ -19,6 +19,57 @@ - + diff --git a/projects/v3/src/styles.scss b/projects/v3/src/styles.scss index 42fe3266fe..c2d93cd889 100644 --- a/projects/v3/src/styles.scss +++ b/projects/v3/src/styles.scss @@ -569,3 +569,20 @@ quill-editor .ql-toolbar.ql-snow { margin-right: 8px !important; } } + +// accessWidget is loaded only on non-local hosts. Keep its custom trigger out +// of the local UI and reserve enough room for the vendor trigger if a remote +// accessiBe configuration overrides hideTrigger. +body:not(.accessibility-widget-enabled) app-personalised-header .accessibility-btn { + display: none; +} + +body.accessibility-widget-enabled app-chat-room ion-row.action-buttons { + padding-inline-end: 64px; +} + +@media (min-width: 768px) { + body.accessibility-widget-enabled app-chat-room ion-row.action-buttons { + padding-inline-end: 88px; + } +} From f6c4c7a8aba69e02353ceb49462ffaef6bab472b Mon Sep 17 00:00:00 2001 From: trtshen Date: Fri, 7 Aug 2026 14:46:40 +0800 Subject: [PATCH 11/14] [CORE-6152] brought back --- docs/accessibility/WCAG_CHECKLIST.md | 16 +++++++--------- .../personalised-header.component.html | 11 ----------- .../personalised-header.component.scss | 1 - .../personalised-header.component.spec.ts | 13 ------------- projects/v3/src/index.html | 2 +- projects/v3/src/styles.scss | 9 ++------- 6 files changed, 10 insertions(+), 42 deletions(-) diff --git a/docs/accessibility/WCAG_CHECKLIST.md b/docs/accessibility/WCAG_CHECKLIST.md index cae2af1849..3458bfa9b7 100644 --- a/docs/accessibility/WCAG_CHECKLIST.md +++ b/docs/accessibility/WCAG_CHECKLIST.md @@ -123,9 +123,8 @@ This checklist verifies compliance with WCAG 2.2 Level AA standards for the V3 I - [x] **COMPLETED**: Added CSS to prevent focus obscuring (scroll-margin: 4px on focus-visible elements) - [x] Sticky headers/footers have proper z-index (ion-header and ion-footer set to z-index: 1000) - [x] Modals/overlays configured with backdrop opacity (ion-modal has --backdrop-opacity: 0.4) -- [x] **COMPLETED**: Replaced accessWidget's production floating trigger with a keyboard-accessible custom trigger in the personalised header -- [x] **COMPLETED**: Reserved production-only inline space in the chat action row so a vendor-overridden floating trigger cannot obscure Attach or Send -- [ ] **RETEST ON DEPLOYED HOST**: Verify the custom trigger opens accessWidget and the chat controls remain unobscured at desktop, tablet, and mobile viewports +- [x] **COMPLETED**: Reserved production-only inline space in the chat action row so accessWidget's floating trigger cannot obscure Attach or Send +- [ ] **RETEST ON DEPLOYED HOST**: Verify accessWidget remains available and the chat controls remain unobscured at desktop, tablet, and mobile viewports #### 2.4.13 Focus Appearance (Minimum) (Level AA) - NEW in 2.2 - [x] Focus indicators have at least 2px outline (implemented in global.scss) @@ -397,7 +396,7 @@ This checklist verifies compliance with WCAG 2.2 Level AA standards for the V3 I 7. Test with keyboard: Tab to element, tooltip should appear; ESC should dismiss it #### 5. Focus Not Obscured (WCAG 2.4.11) -**Fixed in:** `global.scss`, `styles.scss`, `personalised-header.component.html`, and `index.html` +**Fixed in:** `global.scss`, `styles.scss`, and `index.html` **Retest Instructions:** 1. Navigate to any page with sticky header/footer @@ -407,11 +406,10 @@ This checklist verifies compliance with WCAG 2.2 Level AA standards for the V3 I 5. Open DevTools and check computed styles on focused element: - `scroll-margin: 4px` should be present - `z-index` on headers/footers should be 1000 -6. On a deployed non-local host, Tab to the header button labelled "Open accessibility options" -7. **VERIFY**: Enter and Space open accessWidget and the button remains visible with support, notification, and profile controls -8. Navigate to Messages and select a writable chat room -9. **VERIFY**: Attach and Send remain visible and clickable at 2048x1048, 1366x768, 1024x768, and 390x844 -10. **VERIFY**: If accessiBe overrides `hideTrigger`, the remaining floating trigger does not obscure the chat action row +6. On a deployed non-local host, verify accessWidget's floating trigger remains visible and opens the widget +7. Navigate to Messages and select a writable chat room +8. **VERIFY**: Attach and Send remain visible and clickable at 2048x1048, 1366x768, 1024x768, and 390x844 +9. **VERIFY**: The floating trigger does not obscure the chat action row #### 6. Text Spacing Support (WCAG 1.4.12) **Fixed in:** `global.scss` diff --git a/projects/v3/src/app/personalised-header/personalised-header.component.html b/projects/v3/src/app/personalised-header/personalised-header.component.html index 3db5ace695..799de5f020 100644 --- a/projects/v3/src/app/personalised-header/personalised-header.component.html +++ b/projects/v3/src/app/personalised-header/personalised-header.component.html @@ -25,17 +25,6 @@ - - - - { it('should create', () => { expect(component).toBeTruthy(); }); - - it('should expose the accessWidget custom trigger before notifications', () => { - const accessibilityButton: HTMLElement = fixture.nativeElement.querySelector('.accessibility-btn'); - const notificationButton: HTMLElement = fixture.nativeElement.querySelector('.notify-btn'); - const icon: HTMLElement = accessibilityButton.querySelector('ion-icon'); - - expect(accessibilityButton).toBeTruthy(); - expect(accessibilityButton.getAttribute('aria-label')).toBe('Open accessibility options'); - expect(accessibilityButton.getAttribute('data-acsb-custom-trigger')).toBe('true'); - expect(icon.getAttribute('name')).toBe('accessibility-outline'); - expect(accessibilityButton.compareDocumentPosition(notificationButton) & Node.DOCUMENT_POSITION_FOLLOWING) - .toBeTruthy(); - }); }); diff --git a/projects/v3/src/index.html b/projects/v3/src/index.html index 8b10cc5f88..0a33b0f7ea 100644 --- a/projects/v3/src/index.html +++ b/projects/v3/src/index.html @@ -43,7 +43,7 @@ statementLink: '', footerHtml: '', hideMobile: false, - hideTrigger: true, + hideTrigger: false, disableBgProcess: false, language: 'en', position: 'left', diff --git a/projects/v3/src/styles.scss b/projects/v3/src/styles.scss index c2d93cd889..db31281897 100644 --- a/projects/v3/src/styles.scss +++ b/projects/v3/src/styles.scss @@ -570,13 +570,8 @@ quill-editor .ql-toolbar.ql-snow { } } -// accessWidget is loaded only on non-local hosts. Keep its custom trigger out -// of the local UI and reserve enough room for the vendor trigger if a remote -// accessiBe configuration overrides hideTrigger. -body:not(.accessibility-widget-enabled) app-personalised-header .accessibility-btn { - display: none; -} - +// accessWidget is loaded only on non-local hosts. Reserve enough room in the +// chat composer for its floating trigger without changing unrelated pages. body.accessibility-widget-enabled app-chat-room ion-row.action-buttons { padding-inline-end: 64px; } From 4d07f453690fc85dd7fa595c7046086037f22256 Mon Sep 17 00:00:00 2001 From: trtshen Date: Fri, 7 Aug 2026 15:53:32 +0800 Subject: [PATCH 12/14] [core-6152] mobiley offset --- projects/v3/src/index.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/v3/src/index.html b/projects/v3/src/index.html index 0a33b0f7ea..e660258fc8 100644 --- a/projects/v3/src/index.html +++ b/projects/v3/src/index.html @@ -61,7 +61,7 @@ triggerPositionX: 'right', triggerPositionY: 'bottom', triggerOffsetX: 10, - triggerOffsetY: 10, + triggerOffsetY: 88, triggerRadius: '50%' } }); From 0030e6dfa1fb6e2f6cfdeb10f524bc3c3e3bc082 Mon Sep 17 00:00:00 2001 From: trtshen Date: Wed, 12 Aug 2026 10:17:27 +0800 Subject: [PATCH 13/14] [CORE-8316] signed url and preview url are different from uppy uploader --- docs/docs.md | 1 + .../CORE-8316-immediate-attachment-preview.md | 61 ++++++++++++++ .../chat-room/chat-room.component.spec.ts | 83 +++++++++++++++++++ .../chat/chat-room/chat-room.component.ts | 12 +-- .../v3/src/app/services/pusher.service.ts | 6 +- 5 files changed, 156 insertions(+), 7 deletions(-) create mode 100644 docs/fixes/CORE-8316-immediate-attachment-preview.md diff --git a/docs/docs.md b/docs/docs.md index c0566222ae..5cace35657 100644 --- a/docs/docs.md +++ b/docs/docs.md @@ -32,6 +32,7 @@ This is practera documentation with more informations. - [Slider Rating Implementation](./features/slider-rating-implementation.md) ### Fixes +- [CORE-8316 Immediate Message Attachment Preview](./fixes/CORE-8316-immediate-attachment-preview.md) - [CORE-7942 Whitespace Fix](./fixes/CORE-7942-whitespace-fix.md) - [CORE-8002 Pulse Check Workflow](./fixes/CORE-8002-pulsecheck-workflow.md) - [CORE-8166/8167 Pagination Answer Persistence](./fixes/CORE-8166-8167-pagination-answer-persistence.md) diff --git a/docs/fixes/CORE-8316-immediate-attachment-preview.md b/docs/fixes/CORE-8316-immediate-attachment-preview.md new file mode 100644 index 0000000000..2588aa609a --- /dev/null +++ b/docs/fixes/CORE-8316-immediate-attachment-preview.md @@ -0,0 +1,61 @@ +--- +status: stable +authority: reference +scope: v3 +last_reviewed: 2026-08-12 +supersedes: none +--- + +# CORE-8316: Immediate Message Attachment Preview + +## Failure + +When a user sent an attachment, another user in the same chat received the new +message immediately through Pusher but could not render or preview the file +until reloading the page. + +The uploader provides the canonical CDN path immediately after upload. The +`createChatLog` mutation then returns the stored file with an authorized URL. +The sender rendered that API response, but the Pusher event used the original +uploader object instead. Recipients therefore received the canonical path +without the authorization query parameters required to load the file. + +Reloading appeared to fix the problem because `getMessageList()` fetched the +message through GraphQL, which returned an authorized file URL. + +## Required real-time contract + +After `createChatLog` succeeds, all local rendering and real-time delivery must +use the mutation response as the source of truth: + +- `response.file` is added to the sender's message list. +- `response.file` is included in the `client-chat-new-message` event. +- The pre-mutation uploader object is used only as the mutation input. + +The Pusher attachment payload is either `null` or an object containing `name`, +`type`, and the API-returned `url`. + +## Pre-send preview contract + +The composer must display a newly uploaded image before the message is sent. +At this point the canonical CDN URL may not be immediately readable, so the +selected attachment uses this fallback order only for its temporary preview: + +1. `directUrl` +2. canonical `url` +3. TUS `uploadUrl` + +The message mutation continues to receive the canonical `url`; the direct URL +must not replace the value persisted with the chat message. + +## Verification + +Automated coverage sends an attachment with an unsigned uploader URL while the +mocked message API returns a signed URL. It verifies that the Pusher event uses +the API-returned file and never the uploader object. Separate coverage verifies +that a selected attachment previews `directUrl` while retaining the canonical +URL for the outgoing message. + +For staging verification, keep two users in the same direct-message channel, +send an image from one browser, and open it immediately in the receiving +browser without reloading. Both the inline image and preview modal must render. diff --git a/projects/v3/src/app/pages/chat/chat-room/chat-room.component.spec.ts b/projects/v3/src/app/pages/chat/chat-room/chat-room.component.spec.ts index 6b4197c8ec..82f2d14d9a 100644 --- a/projects/v3/src/app/pages/chat/chat-room/chat-room.component.spec.ts +++ b/projects/v3/src/app/pages/chat/chat-room/chat-room.component.spec.ts @@ -357,6 +357,89 @@ describe('ChatRoomComponent', () => { preview: undefined }); }); + + it('should broadcast the signed attachment returned by the message API', () => { + const uploadedAttachment = { + bucket: 'chat', + path: '/uploads/image.png', + name: 'image.png', + url: 'https://file.example.com/files/chat/image.png', + extension: 'png', + type: 'image/png', + size: 1024, + preview: 'https://file.example.com/files/chat/image.png', + }; + const signedFile = { + name: 'image.png', + type: 'image/png', + url: 'https://file.example.com/files/chat/image.png?Signature=signed', + }; + const saveMessageRes = { + uuid: 'attachment-message-uuid', + isSender: true, + message: '', + file: signedFile, + created: '2026-08-12 09:30:00', + sentAt: '2026-08-12 09:30:00', + senderUuid: 'sender-uuid', + senderName: 'Sender', + senderRole: 'participant', + senderAvatar: null, + sender: { + uuid: 'sender-uuid', + name: 'Sender', + role: 'participant', + avatar: null, + }, + }; + + component.channelUuid = 'channel-uuid'; + component.chatChannel.pusherChannel = 'private-chat-channel'; + component.messagePageCursor = 'existing-cursor'; + component.selectedAttachments = [uploadedAttachment]; + chatServiceSpy.postNewMessage.and.returnValue(of(saveMessageRes)); + pusherSpy.triggerSendMessage.calls.reset(); + + component.sendMessage(); + + expect(pusherSpy.triggerSendMessage).toHaveBeenCalledTimes(1); + expect(pusherSpy.triggerSendMessage).toHaveBeenCalledWith( + 'private-chat-channel', + jasmine.objectContaining({ + uuid: saveMessageRes.uuid, + file: signedFile, + }) + ); + expect(pusherSpy.triggerSendMessage.calls.mostRecent().args[1].file) + .not.toBe(uploadedAttachment); + }); + }); + + describe('when testing addAttachment()', () => { + it('should preview the direct URL while retaining the canonical message URL', () => { + const upload = { + name: 'cyberpunk.png', + url: 'https://cdn.example.com/files/chat/cyberpunk.png', + directUrl: 'https://uploads.example.com/chat/cyberpunk.png?token=direct', + extension: 'png', + type: 'image/png', + size: 1024, + bucket: 'chat', + path: '/uploads/cyberpunk.png', + tus: { + uploadUrl: 'https://uploads.example.com/tus/cyberpunk.png', + }, + } as any; + + component.addAttachment(upload); + + expect(component.selectedAttachments[0]).toEqual( + jasmine.objectContaining({ + url: upload.url, + preview: upload.directUrl, + }) + ); + }); }); describe('when testing getAvatarClass()', () => { diff --git a/projects/v3/src/app/pages/chat/chat-room/chat-room.component.ts b/projects/v3/src/app/pages/chat/chat-room/chat-room.component.ts index 8d2704d349..a5cb09c979 100644 --- a/projects/v3/src/app/pages/chat/chat-room/chat-room.component.ts +++ b/projects/v3/src/app/pages/chat/chat-room/chat-room.component.ts @@ -561,7 +561,7 @@ export class ChatRoomComponent implements OnInit, OnDestroy, AfterViewInit { .pipe(takeUntil(this.destroy$)) .subscribe( (response) => { - this.afterEventEmission(response, attachment); + this.afterEventEmission(response); this.removeSelectAttachment(attachment); }, (error) => { @@ -574,8 +574,8 @@ export class ChatRoomComponent implements OnInit, OnDestroy, AfterViewInit { } // series of after event emission actions (triggered after sending message) - afterEventEmission(response, attachment?) { - this.triggerPusherEvent(response, attachment); + afterEventEmission(response) { + this.triggerPusherEvent(response); this.updateListData(response); this.utils.broadcastEvent("chat:info-update", true); this._scrollToBottom(); @@ -583,13 +583,13 @@ export class ChatRoomComponent implements OnInit, OnDestroy, AfterViewInit { } // trigger pusher event with file response - triggerPusherEvent(response, file?: FileResponse) { + triggerPusherEvent(response) { const pusherData: SendMessageParam = { channelUuid: this.channelUuid, uuid: response.uuid, isSender: response.isSender, message: response.message, - file: file || response.file, + file: response.file, created: response.created, senderUuid: response.senderUuid, senderName: response.senderName, @@ -1058,7 +1058,7 @@ export class ChatRoomComponent implements OnInit, OnDestroy, AfterViewInit { // tusd custom fields bucket: uppyRes.bucket, path: uppyRes.path, - preview: uppyRes.url || uppyRes.tus.uploadUrl, + preview: uppyRes.directUrl || uppyRes.url || uppyRes.tus.uploadUrl, }); } diff --git a/projects/v3/src/app/services/pusher.service.ts b/projects/v3/src/app/services/pusher.service.ts index d8657a0eaa..fa74068ea8 100644 --- a/projects/v3/src/app/services/pusher.service.ts +++ b/projects/v3/src/app/services/pusher.service.ts @@ -18,7 +18,11 @@ export interface SendMessageParam { channelUuid: string; uuid: string; message: string; - file: string; + file: { + name: string; + type: string; + url: string; + } | string | null; isSender: boolean; created: string; senderUuid: string; From 3c4f098ee43ae1ea49a3d2857960954df3b2d996 Mon Sep 17 00:00:00 2001 From: trtshen Date: Wed, 12 Aug 2026 10:50:03 +0800 Subject: [PATCH 14/14] [CORE-8136] preview with proper url --- .../CORE-8316-immediate-attachment-preview.md | 7 +++++- .../chat-preview/chat-preview.component.html | 6 ++--- .../chat-preview.component.spec.ts | 23 +++++++++++++++++++ .../chat-preview/chat-preview.component.ts | 4 ++++ 4 files changed, 36 insertions(+), 4 deletions(-) diff --git a/docs/fixes/CORE-8316-immediate-attachment-preview.md b/docs/fixes/CORE-8316-immediate-attachment-preview.md index 2588aa609a..ace61d584c 100644 --- a/docs/fixes/CORE-8316-immediate-attachment-preview.md +++ b/docs/fixes/CORE-8316-immediate-attachment-preview.md @@ -48,13 +48,18 @@ selected attachment uses this fallback order only for its temporary preview: The message mutation continues to receive the canonical `url`; the direct URL must not replace the value persisted with the chat message. +Both the inline composer thumbnail and the preview modal render the temporary +`preview` URL. The modal download action continues to use the canonical `url`. + ## Verification Automated coverage sends an attachment with an unsigned uploader URL while the mocked message API returns a signed URL. It verifies that the Pusher event uses the API-returned file and never the uploader object. Separate coverage verifies that a selected attachment previews `directUrl` while retaining the canonical -URL for the outgoing message. +URL for the outgoing message. The preview modal coverage also verifies that its +rendered media uses the immediate preview URL and falls back to the canonical +URL for attachments that have already been sent. For staging verification, keep two users in the same direct-message channel, send an image from one browser, and open it immediately in the receiving diff --git a/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.html b/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.html index 054c5697a5..c63617a0c3 100644 --- a/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.html +++ b/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.html @@ -29,8 +29,8 @@
- - uploaded attachment preview + + uploaded attachment preview @@ -40,7 +40,7 @@ controlsList="nodownload" preload="metadata" playsinline - [src]="file.url" + [src]="previewUrl" (error)="handleVideoError($event)" >

diff --git a/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.spec.ts b/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.spec.ts index 75d262c72b..4a55855f75 100644 --- a/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.spec.ts +++ b/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.spec.ts @@ -49,6 +49,29 @@ describe('ChatPreviewComponent', () => { expect(component.file.url).toBe(TEST_URL); }); + describe('previewUrl', () => { + it('should render the immediate preview URL when it is available', () => { + const directUrl = 'https://uploads.example.com/chat/image.png?token=direct'; + component.file = { + type: 'image/png', + url: 'https://cdn.example.com/chat/image.png', + preview: directUrl, + }; + + fixture.detectChanges(); + + const image = fixture.nativeElement.querySelector('img') as HTMLImageElement; + expect(component.previewUrl).toBe(directUrl); + expect(image.src).toBe(directUrl); + }); + + it('should fall back to the canonical URL for sent attachments', () => { + component.file = { url: TEST_URL }; + + expect(component.previewUrl).toBe(TEST_URL); + }); + }); + describe('download()', () => { it('should open and download from a URL', () => { spyOn(window, 'open'); diff --git a/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.ts b/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.ts index 9609a2bd1e..5c3ef95fe4 100644 --- a/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.ts +++ b/projects/v3/src/app/pages/chat/chat-preview/chat-preview.component.ts @@ -16,6 +16,10 @@ export class ChatPreviewComponent { public sanitizer: DomSanitizer ) {} + get previewUrl(): string { + return this.file?.preview || this.file?.url; + } + download(keyboardEvent?: KeyboardEvent) { if (keyboardEvent && (keyboardEvent?.code === 'Space' || keyboardEvent?.code === 'Enter')) { keyboardEvent.preventDefault();