From 9ff93d4bfbaf802c746d9a5607f830040cc88e1a Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Sun, 27 Sep 2026 19:58:35 +1000 Subject: [PATCH 1/3] feat(ui): page redesign and UI fixes Brings the T2 2026 ui work from ontrack-features-t2-2026 11.0.x (reviewed and merged work) onto thoth-tech 11.0.x. Co-authored-by: maplefoxgit Co-authored-by: David Tenni Co-authored-by: Sam Co-authored-by: sakethsram888 Co-authored-by: baohuy-tran Co-authored-by: owensmac Co-authored-by: Maple Fox Co-authored-by: The Herald Sun Co-authored-by: Tan Tai Co-authored-by: Swyam Khare Co-authored-by: Sujay-Deakin Co-authored-by: Cilly Leang Co-authored-by: UmedaRanuluge Co-authored-by: Thirus224849242 Co-authored-by: owens-hub-git Co-authored-by: SandilBandara Co-authored-by: Kimsreng Co-authored-by: Gaurav Myana Co-authored-by: tranhuy110 Co-authored-by: shalitha99 Co-authored-by: Shaashwat3 Co-authored-by: SanjanaBottu Co-authored-by: mudith-perera Co-authored-by: lgr2002 Co-authored-by: Labibatanjim Co-authored-by: JOSHUA ERICKSON Co-authored-by: Jake Mansfield Co-authored-by: Freddy Co-authored-by: Duy Nguyen Co-authored-by: Davie --- .gitignore | 2 + .prettierignore | 4 - angular.json | 5 +- docs/ISSUE-1290-testing-strategy.md | 130 +++++ docs/cross-project-ui.md | 68 +++ docs/cross-unit-dashboard.md | 36 ++ .../edit-profile/edit-profile.component.html | 12 +- .../edit-profile/edit-profile.component.scss | 3 + .../activity-type-list.component.html | 36 +- .../activity-type-list.component.ts | 25 +- .../campus-list/campus-list.component.ts | 24 +- .../institution-settings.component.html | 38 +- .../institution-settings.component.scss | 3 - .../overseer-image-list.component.html | 44 +- .../overseer-image-list.component.ts | 1 - .../teaching-period-list.component.spec.ts | 25 +- .../teaching-period-list.component.ts | 7 +- src/app/admin/states/users/users.component.ts | 3 +- .../tii-action-log.component.html | 16 +- .../tii-action-log.component.spec.ts | 59 ++ .../tii-action-log.component.ts | 1 - src/app/api/models/course-flow.ts | 48 ++ src/app/api/models/doubtfire-model.ts | 5 + src/app/api/models/project.ts | 28 +- src/app/api/models/task.ts | 37 +- src/app/api/models/unit-role.ts | 2 +- src/app/api/models/unit.spec.ts | 59 ++ src/app/api/models/unit.ts | 2 +- src/app/api/models/user/user.ts | 6 +- .../api/services/course-flow.service.spec.ts | 210 +++++++ src/app/api/services/course-flow.service.ts | 56 ++ .../api/services/spec/task.service.spec.ts | 109 ++++ .../api/services/spec/user.service.spec.ts | 208 ++++--- .../api/services/task-similarity.service.ts | 1 - src/app/api/services/task.service.ts | 13 + src/app/api/services/user.service.ts | 17 +- src/app/app.component.html | 10 +- src/app/app.routes.spec.ts | 32 ++ src/app/app.routes.ts | 171 +++++- .../audio-player/audio-player.component.html | 2 +- .../audio-player.component.spec.ts | 34 ++ .../audio-player/audio-player.component.ts | 8 +- .../audio-comment-recorder.html | 23 +- .../audio-comment-recorder.ts | 11 +- .../audio/base-audio-recorder.ts | 32 +- .../collection-empty-states.component.spec.ts | 203 +++++++ .../empty-state/empty-state.component.html | 12 + .../empty-state/empty-state.component.scss | 3 + .../empty-state/empty-state.component.spec.ts | 72 +++ .../empty-state/empty-state.component.ts | 28 + .../entity-form/entity-form.component.spec.ts | 44 ++ .../entity-form/entity-form.component.ts | 10 +- src/app/common/filters/filters.coffee | 320 ----------- src/app/common/guards/role-whitelist.guard.ts | 19 +- .../guards/spec/role-whitelist.guard.spec.ts | 456 +++++++++++++++ src/app/common/header/header.component.html | 98 +++- .../common/header/header.component.spec.ts | 339 ++++++++++- src/app/common/header/header.component.ts | 94 ++- .../task-dropdown.component.html | 7 +- .../unit-dropdown.component.html | 17 +- .../unit-dropdown.component.spec.ts | 4 +- .../unit-dropdown/unit-dropdown.component.ts | 8 +- .../learning-outcome-editor.component.spec.ts | 41 ++ .../learning-outcome-editor.component.ts | 14 +- .../confirmation-modal.component.html | 2 +- .../extension-modal.component.spec.ts | 21 + .../extension-modal.component.ts | 6 +- .../scorm-extension-modal.component.ts | 6 +- src/app/common/pipes/marked.pipe.spec.ts | 19 + src/app/common/pipes/marked.pipe.ts | 2 +- src/app/common/pipes/safe.pipe.spec.ts | 21 +- .../pwa-connection-status.component.html | 8 + .../pwa-connection-status.component.scss | 13 + .../pwa-connection-status.component.spec.ts | 74 +++ .../pwa-connection-status.component.ts | 22 + .../pwa/pwa-install-button.component.html | 3 + .../pwa/pwa-install-button.component.spec.ts | 54 ++ .../pwa/pwa-install-button.component.ts | 25 + .../pwa/pwa-install-dialog.component.html | 105 ++++ .../pwa/pwa-install-dialog.component.scss | 43 ++ .../pwa/pwa-install-dialog.component.spec.ts | 98 ++++ .../pwa/pwa-install-dialog.component.ts | 20 + .../common/pwa/pwa-install.service.spec.ts | 251 ++++++++ src/app/common/pwa/pwa-install.service.ts | 143 +++++ src/app/common/services/emoji.service.ts | 6 +- .../http-authentication.interceptor.spec.ts | 73 +++ src/app/common/services/http-error-context.ts | 6 + .../services/http-error.interceptor.spec.ts | 80 +++ .../common/services/http-error.interceptor.ts | 58 +- .../skeleton-loader.component.html | 55 ++ .../skeleton-loader.component.scss | 3 + .../skeleton-loader.component.spec.ts | 105 ++++ .../skeleton-loader.component.ts | 47 ++ .../constants/doubtfire-constants.spec.ts | 281 +++++++++ .../config/constants/doubtfire-constants.ts | 122 +++- .../coursemap/course-flow-checks.spec.ts | 86 +++ .../coursemap/course-flow-checks.ts | 52 ++ .../coursemap/course-flow-dirty.guard.ts | 8 + .../coursemap/coursemap.component.html | 351 +++++++++++ .../coursemap/coursemap.component.scss | 333 +++++++++++ .../coursemap/coursemap.component.spec.ts | 502 ++++++++++++++++ .../coursemap/coursemap.component.ts | 529 +++++++++++++++++ src/app/doubtfire-angular.module.ts | 67 ++- .../states/timeout/timeout.component.html | 2 + .../states/timeout/timeout.component.spec.ts | 26 + .../states/timeout/timeout.component.ts | 8 + .../accept-eula/accept-eula.component.spec.ts | 71 ++- .../eula/accept-eula/accept-eula.component.ts | 23 +- .../group-set-manager.component.html | 2 +- .../group-set-manager.component.spec.ts | 53 ++ .../group-set-manager.component.ts | 22 +- src/app/home/states/home/home.component.html | 95 +-- .../home/states/home/home.component.spec.ts | 63 ++ src/app/home/states/home/home.component.ts | 28 +- .../project-progress-dashboard.component.html | 4 +- .../progress-dashboard.component.html | 43 +- .../progress-dashboard.component.spec.ts | 229 ++++++++ .../progress-dashboard.component.ts | 104 +++- .../task-planner-card.component.html | 16 +- .../task-planner-card.component.spec.ts | 534 +++++++++++++++++ .../task-description-card.component.html | 44 +- .../task-description-card.component.spec.ts | 349 +++++++++++ .../task-description-card.component.ts | 115 +++- .../task-similarity-view.component.ts | 2 +- .../tutor-notes-view.component.html | 2 +- .../project-dashboard.component.html | 304 ++++++---- .../project-dashboard.component.scss | 155 ++++- .../project-dashboard.component.spec.ts | 402 +++++++++++++ .../project-dashboard.component.ts | 225 +++++++- .../project-dashboard.mobile.spec.ts | 336 +++++++++++ .../discussion-prompts.component.html | 53 +- .../discussion-prompts.component.ts | 19 +- .../project-groups.component.html | 4 +- .../states/index/global-state.service.spec.ts | 152 +++++ .../states/index/global-state.service.ts | 56 +- .../states/plan/project-plan.component.html | 92 +-- .../plan/project-plan.component.spec.ts | 92 +++ .../project-root-state.component.spec.ts | 24 + .../states/project-root-state.component.ts | 10 +- .../staff-notes/staff-notes.component.spec.ts | 89 ++- .../tutor-discussion.component.html | 443 +++++++------- .../states/tutorials/tutorials.component.html | 196 +++---- .../tutorials/tutorials.component.spec.ts | 57 ++ .../states/tutorials/tutorials.component.ts | 17 +- .../student-onboarding-target.ts | 23 + .../student-onboarding.component.html | 127 ++++ .../student-onboarding.component.scss | 85 +++ .../student-onboarding.component.spec.ts | 219 +++++++ .../student-onboarding.component.ts | 146 +++++ .../student-onboarding.service.spec.ts | 441 ++++++++++++++ .../student-onboarding.service.ts | 373 ++++++++++++ .../student-onboarding.steps.ts | 59 ++ .../task-comment-composer.component.html | 88 +-- .../task-comment-composer.component.scss | 99 +++- .../task-comment-composer.component.spec.ts | 345 +++++++++++ .../extension-comment.component.html | 8 +- .../extension-comment.component.ts | 2 +- ...intelligent-discussion-player.component.ts | 2 +- .../pdf-image-comment.component.html | 17 +- .../scorm-extension-comment.component.ts | 2 +- .../task-comments-viewer.component.spec.ts | 249 +++++++- .../task-comments-viewer.component.ts | 54 +- .../communication-schedules.component.html | 8 +- .../d2l-unit-details-form.component.ts | 2 +- .../unit-group-set-editor.component.ts | 27 +- .../unit-staff-editor.component.html | 18 +- .../unit-staff-editor.component.spec.ts | 42 ++ .../unit-staff-editor.component.ts | 8 +- .../unit-students-editor.component.html | 46 +- .../unit-students-editor.component.ts | 9 +- .../task-definition-editor.component.html | 62 +- .../task-definition-editor.component.scss | 31 + .../unit-task-editor.component.html | 81 ++- .../unit-task-editor.component.scss | 26 + .../unit-task-editor.component.spec.ts | 111 ++++ .../unit-task-editor.component.ts | 45 ++ .../unit-tutorials-list.component.html | 65 ++- .../unit-tutorials-list.component.ts | 2 +- .../unit-tutorials-manager.component.html | 14 +- .../edit/unit-admin-state.component.html | 72 +-- .../unit-groups/unit-groups.component.html | 34 +- .../unit-groups/unit-groups.component.ts | 21 +- .../states/rollover/rollover.component.html | 4 +- .../rollover/rollover.component.spec.ts | 94 +++ .../states/rollover/rollover.component.ts | 19 +- .../students-list.component.html | 75 +-- .../students-list.component.spec.ts | 248 ++++++++ .../students-list/students-list.component.ts | 61 +- .../states/tasks/definition/definition.coffee | 31 - .../confirm-moderation-modal.component.html | 4 +- .../staff-task-list.component.ts | 103 +++- .../tasks/inbox/inbox.component.spec.ts | 2 - .../states/tasks/inbox/inbox.component.ts | 7 +- .../unit-task-inbox-state.component.spec.ts | 186 ++++++ .../inbox/unit-task-inbox-state.component.ts | 49 +- .../task-details-view.component.html | 2 +- .../task-sheet-view.component.html | 2 +- .../unit-task-list.component.html | 196 +++++-- .../unit-task-list.component.spec.ts | 544 +++++++++++++++++- .../unit-task-list.component.ts | 210 +++++-- .../task-viewer-state.component.html | 10 +- .../task-viewer-state.component.ts | 16 +- src/app/units/unit-root-state.component.html | 2 +- .../units/unit-root-state.component.spec.ts | 90 +++ src/app/units/unit-root-state.component.ts | 30 +- .../ngx-charts-compatibility.spec.ts | 67 +++ .../summary-task-status-scatter.coffee | 29 - ...summary-task-status-scatter.component.html | 65 +++ ...mary-task-status-scatter.component.spec.ts | 74 +++ .../summary-task-status-scatter.component.ts | 65 +++ .../target-grade-pie-chart.coffee | 48 -- .../target-grade-pie-chart.component.html | 39 ++ .../target-grade-pie-chart.component.spec.ts | 59 ++ .../target-grade-pie-chart.component.ts | 50 ++ .../task-completion-box-plot.coffee | 66 --- .../task-completion-box-plot.component.html | 61 ++ ...task-completion-box-plot.component.spec.ts | 57 ++ .../task-completion-box-plot.component.ts | 62 ++ .../task-status-pie-chart.coffee | 46 -- src/app/visualisations/visualisation.tpl.html | 3 - src/app/visualisations/visualisations.coffee | 61 -- src/app/welcome/welcome.component.html | 18 +- src/environments/environment.ts | 14 + 223 files changed, 15399 insertions(+), 2249 deletions(-) create mode 100644 docs/ISSUE-1290-testing-strategy.md create mode 100644 docs/cross-project-ui.md create mode 100644 docs/cross-unit-dashboard.md create mode 100644 src/app/api/models/course-flow.ts create mode 100644 src/app/api/models/unit.spec.ts create mode 100644 src/app/api/services/course-flow.service.spec.ts create mode 100644 src/app/api/services/course-flow.service.ts create mode 100644 src/app/api/services/spec/task.service.spec.ts create mode 100644 src/app/app.routes.spec.ts create mode 100644 src/app/common/audio-player/audio-player.component.spec.ts create mode 100644 src/app/common/empty-state/collection-empty-states.component.spec.ts create mode 100644 src/app/common/empty-state/empty-state.component.html create mode 100644 src/app/common/empty-state/empty-state.component.scss create mode 100644 src/app/common/empty-state/empty-state.component.spec.ts create mode 100644 src/app/common/empty-state/empty-state.component.ts create mode 100644 src/app/common/entity-form/entity-form.component.spec.ts delete mode 100644 src/app/common/filters/filters.coffee create mode 100644 src/app/common/guards/spec/role-whitelist.guard.spec.ts create mode 100644 src/app/common/learning-outcome-editor/learning-outcome-editor.component.spec.ts create mode 100644 src/app/common/modals/extension-modal/extension-modal.component.spec.ts create mode 100644 src/app/common/pwa-connection/pwa-connection-status.component.html create mode 100644 src/app/common/pwa-connection/pwa-connection-status.component.scss create mode 100644 src/app/common/pwa-connection/pwa-connection-status.component.spec.ts create mode 100644 src/app/common/pwa-connection/pwa-connection-status.component.ts create mode 100644 src/app/common/pwa/pwa-install-button.component.html create mode 100644 src/app/common/pwa/pwa-install-button.component.spec.ts create mode 100644 src/app/common/pwa/pwa-install-button.component.ts create mode 100644 src/app/common/pwa/pwa-install-dialog.component.html create mode 100644 src/app/common/pwa/pwa-install-dialog.component.scss create mode 100644 src/app/common/pwa/pwa-install-dialog.component.spec.ts create mode 100644 src/app/common/pwa/pwa-install-dialog.component.ts create mode 100644 src/app/common/pwa/pwa-install.service.spec.ts create mode 100644 src/app/common/pwa/pwa-install.service.ts create mode 100644 src/app/common/services/http-authentication.interceptor.spec.ts create mode 100644 src/app/common/services/http-error-context.ts create mode 100644 src/app/common/services/http-error.interceptor.spec.ts create mode 100644 src/app/common/skeleton-loader/skeleton-loader.component.html create mode 100644 src/app/common/skeleton-loader/skeleton-loader.component.scss create mode 100644 src/app/common/skeleton-loader/skeleton-loader.component.spec.ts create mode 100644 src/app/common/skeleton-loader/skeleton-loader.component.ts create mode 100644 src/app/config/constants/doubtfire-constants.spec.ts create mode 100644 src/app/courseflow/coursemap/course-flow-checks.spec.ts create mode 100644 src/app/courseflow/coursemap/course-flow-checks.ts create mode 100644 src/app/courseflow/coursemap/course-flow-dirty.guard.ts create mode 100644 src/app/courseflow/coursemap/coursemap.component.html create mode 100644 src/app/courseflow/coursemap/coursemap.component.scss create mode 100644 src/app/courseflow/coursemap/coursemap.component.spec.ts create mode 100644 src/app/courseflow/coursemap/coursemap.component.ts create mode 100644 src/app/errors/states/timeout/timeout.component.spec.ts create mode 100644 src/app/groups/group-set-manager/group-set-manager.component.spec.ts create mode 100644 src/app/home/states/home/home.component.spec.ts create mode 100644 src/app/projects/states/dashboard/directives/progress-dashboard/progress-dashboard.component.spec.ts create mode 100644 src/app/projects/states/dashboard/directives/progress-dashboard/task-planner-card/task-planner-card.component.spec.ts create mode 100644 src/app/projects/states/dashboard/directives/task-dashboard/directives/task-description-card/task-description-card.component.spec.ts create mode 100644 src/app/projects/states/dashboard/project-dashboard/project-dashboard.component.spec.ts create mode 100644 src/app/projects/states/dashboard/project-dashboard/project-dashboard.mobile.spec.ts create mode 100644 src/app/projects/states/index/global-state.service.spec.ts create mode 100644 src/app/projects/states/plan/project-plan.component.spec.ts create mode 100644 src/app/projects/states/project-root-state.component.spec.ts create mode 100644 src/app/projects/states/tutorials/tutorials.component.spec.ts create mode 100644 src/app/student-onboarding/student-onboarding-target.ts create mode 100644 src/app/student-onboarding/student-onboarding.component.html create mode 100644 src/app/student-onboarding/student-onboarding.component.scss create mode 100644 src/app/student-onboarding/student-onboarding.component.spec.ts create mode 100644 src/app/student-onboarding/student-onboarding.component.ts create mode 100644 src/app/student-onboarding/student-onboarding.service.spec.ts create mode 100644 src/app/student-onboarding/student-onboarding.service.ts create mode 100644 src/app/student-onboarding/student-onboarding.steps.ts create mode 100644 src/app/tasks/task-comment-composer/task-comment-composer.component.spec.ts create mode 100644 src/app/units/states/edit/directives/unit-staff-editor/unit-staff-editor.component.spec.ts create mode 100644 src/app/units/states/edit/directives/unit-tasks-editor/unit-task-editor.component.spec.ts create mode 100644 src/app/units/states/rollover/rollover.component.spec.ts create mode 100644 src/app/units/states/students-list/students-list.component.spec.ts delete mode 100644 src/app/units/states/tasks/definition/definition.coffee create mode 100644 src/app/units/states/tasks/inbox/unit-task-inbox-state.component.spec.ts create mode 100644 src/app/units/unit-root-state.component.spec.ts create mode 100644 src/app/visualisations/ngx-charts-compatibility.spec.ts delete mode 100644 src/app/visualisations/summary-task-status-scatter.coffee create mode 100644 src/app/visualisations/summary-task-status-scatter/summary-task-status-scatter.component.html create mode 100644 src/app/visualisations/summary-task-status-scatter/summary-task-status-scatter.component.spec.ts create mode 100644 src/app/visualisations/summary-task-status-scatter/summary-task-status-scatter.component.ts delete mode 100644 src/app/visualisations/target-grade-pie-chart.coffee create mode 100644 src/app/visualisations/target-grade-pie-chart/target-grade-pie-chart.component.html create mode 100644 src/app/visualisations/target-grade-pie-chart/target-grade-pie-chart.component.spec.ts create mode 100644 src/app/visualisations/target-grade-pie-chart/target-grade-pie-chart.component.ts delete mode 100644 src/app/visualisations/task-completion-box-plot.coffee create mode 100644 src/app/visualisations/task-completion-box-plot/task-completion-box-plot.component.html create mode 100644 src/app/visualisations/task-completion-box-plot/task-completion-box-plot.component.spec.ts create mode 100644 src/app/visualisations/task-completion-box-plot/task-completion-box-plot.component.ts delete mode 100644 src/app/visualisations/task-status-pie-chart.coffee delete mode 100644 src/app/visualisations/visualisation.tpl.html delete mode 100644 src/app/visualisations/visualisations.coffee diff --git a/.gitignore b/.gitignore index 9912f41615..a125dafc9f 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ .DS_Store build/ dist/ +coverage/ bin/ node_modules/ vendor/ @@ -13,6 +14,7 @@ vendor/ tmp.scss .tscache a.env +.env dist/ .angulardoc.json .nx diff --git a/.prettierignore b/.prettierignore index d541041bd2..2b82fb1787 100644 --- a/.prettierignore +++ b/.prettierignore @@ -7,9 +7,5 @@ e2e CHANGELOG.md JPlag-Report-Viewer/ .github/ -karma/ -Gruntfile.js -karma.conf.js polyfills.ts -test.ts main.ts diff --git a/angular.json b/angular.json index 029aa7eafc..6c5694d24c 100644 --- a/angular.json +++ b/angular.json @@ -79,7 +79,7 @@ } ], "optimization": true, - "outputHashing": "bundles", + "outputHashing": "all", "sourceMap": false, "extractLicenses": true, "serviceWorker": "ngsw-config.json" @@ -87,7 +87,8 @@ "development": { "optimization": false, "extractLicenses": false, - "sourceMap": true + "sourceMap": true, + "serviceWorker": "ngsw-config.json" }, "testing": { "aot": false, diff --git a/docs/ISSUE-1290-testing-strategy.md b/docs/ISSUE-1290-testing-strategy.md new file mode 100644 index 0000000000..c28b993de1 --- /dev/null +++ b/docs/ISSUE-1290-testing-strategy.md @@ -0,0 +1,130 @@ +# Vitest coverage and next integration checks (#1290) + +This records the September 2026 test audit for +[doubtfire-web #1290](https://github.com/doubtfire-lms/doubtfire-web/issues/1290). +The application uses Angular's unit-test builder with Vitest, a jsdom environment, +and the setup file in `src/vitest-setup.ts`. The normal pull request suite does not +require a running API, browser installation, production account, or secrets. + +## Running the checks + +Use the Node version in `.nvmrc` (Node 22.22.3 or later in the supported Node 22 +line), then run: + +```sh +npm ci +npm run test:ci +npm run build +``` + +For a focused iteration, keep Angular's compilation step and select the spec: + +```sh +npm run test:ci -- --include='src/app/common/file-downloader/file-downloader.service.spec.ts' +``` + +Run the whole suite before opening a PR. A focused run cannot detect interactions +with module registration, template compilation, or the rest of the application's +shared fixtures. Existing GitHub `Test CI` and `Node.js CI` workflows run the full +unit suite and application build on pull requests. + +## Recorded validation + +On the FL-22 / #1290 branch based on `11.0.x` commit `a35f2826d`, a clean +`npm ci`, `npm run build`, and the full `npm run test:ci` passed with Node 22.23.2. +The suite contains 146 files and 1107 passing tests. Targeted ESLint also passed. +The runner reports an existing nested `vi.mock` warning in the PDF viewer spec; +it is not a test failure. These results cover this branch, not future merges or +the longer-term browser/API jobs below. + +## Gaps addressed + +The audit found 143 existing spec files under `src/app`. The new tests focus on +observable behaviour in shared code that previously had no dedicated tests: + +| Area | Regression checks | Defect caught | +| -------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| File downloader | Complete and multipart responses, requested byte bounds, missing/malformed ranges, gaps/duplicates, changing totals, interrupted transfers, and a server ignoring Range | Invalid ranges could leave the caller waiting or publish corrupt data; continuation end indexes were one too large; a full response was appended to already received bytes | +| Authentication interceptor | API and LTI credentials, anonymous requests, external/static requests, preservation of caller headers and immutable requests | These cases now protect the shared request boundary | +| Authentication return URL | Blocked browser storage, malformed persisted entries, future timestamps, exact expiry boundary, and one-use consumption | Extends existing unsafe URL, SSO, logout, and expiry checks | + +The downloader tests use `HttpTestingController` to assert the real `HttpClient` +request and response contract, without reaching an actual server. They verify the +assembled Blob contents rather than only checking that a callback ran. Every +case verifies no unmatched requests remain. The auth tests use harmless fixture +tokens and exercise the interceptor/service's public methods. + +## Remaining coverage to prioritise + +This is a risk-based audit, not a claim of complete statement or branch coverage. +Add tests with each change rather than treating creation-only specs as evidence +that every interaction works. + +1. **Authentication refresh concurrency:** several simultaneous expired requests, + failed refresh releasing every waiting subscriber, retry headers, and logout + while a refresh is in flight. `HttpErrorInterceptor` currently has error-body + extraction coverage but needs these stateful scenarios. +2. **Submission state transitions:** combine the upload modal, processing/retry API + response, and selected task route to show that failed uploads retain the user's + context and a successful upload returns to the intended dashboard. +3. **Staff workflows:** task-sheet navigation, tutorial changes, bulk portfolio + operations, and permission refusals. Prefer DOM events and rendered state over + direct calls to private component methods. +4. **Real browser behaviours:** focus restoration, drag/drop, recorded audio, + actual CSS breakpoints, Blob downloads, and service workers. jsdom does not + establish visual layout, accessibility, or browser permission behaviour. +5. **API compatibility:** serialization of deadlines, extensions and upload errors + across the web/API boundary. A frontend mock cannot prove the server emits the + expected data or enforces permissions. + +## Longer-term API test approach + +Keep deterministic frontend contract tests in the normal Vitest suite using +[`provideHttpClientTesting` and `HttpTestingController`](https://angular.dev/guide/http/testing). +Assert request methods, authorization conditions, query/body formats, and error +responses; do not simply return whatever object the component expects. Keep +fixture payloads near the consuming feature and link the corresponding API +endpoint/regression test in its PR. + +Add a separate integration CI job for contracts that require the real API. Boot +pinned web/API/deploy revisions against an isolated disposable database, run +migrations and a small deterministic seed, wait for readiness, then authenticate +through the same endpoints used by the client. Use two fixture students and a +staff member in distinct units to test both allowed requests and cross-user +refusals. Run API model/request tests in the API repository as well; frontend +integration checks supplement them. Tear down the database after each job and +avoid production credentials or public notification delivery. + +Start with three contracts: loading a student's enrolled units; reading the same +task's actual effective deadline; submitting a small fixture file and polling its +processing status. Add an explicit assertion for each relevant 4xx error. This +job should have its own failure report so an API startup failure is not presented +as a frontend assertion failure. + +## Longer-term headless browser approach + +Use two separate layers with a documented purpose: + +- **Browser component tests:** Angular supports + [Vitest browser mode](https://angular.dev/guide/testing) for interactions requiring + a real DOM/browser. Pilot focus and download behaviours before moving large + numbers of otherwise fast jsdom tests. +- **Student journey tests:** a Playwright suite against the built application. + Its [`webServer` configuration](https://playwright.dev/docs/test-webserver) can + manage an isolated local server. For fast PR checks, + [route mocks](https://playwright.dev/docs/network) should cover login, loading a + unit, selecting a task, submitting a file, and the resulting dashboard state. + Add failures for rejected credentials, unavailable units, and failed uploads. + Start with Chromium, then add Firefox/WebKit for browser-specific controls. + +The real API job should reuse the same journeys without route mocks, against the +seeded stack. Keep mocked and real API runs explicitly named; passing a mocked +journey does not prove server compatibility. Use role/name locators and web-first +assertions rather than fixed sleeps. Store a trace/screenshot only on failure, +redact test authorization headers, and keep fixture users' data synthetic. A +service-worker-specific test must run separately from tests that block service +workers for reliable network interception. + +These longer-term jobs are an investigated implementation plan, not new CI +requirements in this PR. They need their own pinned runtime fixtures and review +before becoming required checks. diff --git a/docs/cross-project-ui.md b/docs/cross-project-ui.md new file mode 100644 index 0000000000..f71bcf92e6 --- /dev/null +++ b/docs/cross-project-ui.md @@ -0,0 +1,68 @@ +# Cross-project UI handover + +Tickets: CPD-FB02, CPD-FB03, CPD-UI03–06, PR-XU-12, PR-XU-13 and PR-XU-20. +The API's projects logging and access-control tests are maintained separately. +Calendar features are unchanged. + +## Feedback and filtering + +`has_feedback` maps to `Task.hasFeedback`. `true` means staff feedback exists; +`false` means none exists; missing metadata stays unavailable. No feedback body, +marker note or additional student identity is fetched for a card. The unread +comment count is separate: it cannot establish whether staff feedback is new. +The UI therefore labels it **unread comments**, never **new feedback**. + +The Staff feedback selector combines with status, grade, date range and the +existing global/per-unit searches. Search includes the displayed phrases +`staff feedback available`, `no staff feedback`, `feedback unavailable`, and +`unread comments` where applicable. Searching just `feedback` can match any of +these states; use the selector to ask specifically for available feedback. +Clear all restores all feedback states while preserving per-unit controls. + +The Staff feedback action navigates to +`/projects/:projectId/dashboard/:abbreviation/feedback`, which opens the existing +feedback pane on narrow screens even after comments have been read. The normal +task title links to task details. Neither route bypasses API authorisation. + +## Layout, controls and recovery + +The grid wraps cards in DOM order using a minimum bounded by the available width. +Project name, task completion count, deadline warning and project link remain +available in phone summaries. Comfortable/compact density changes spacing, +not data. Long names wrap. Task controls and focus outlines retain their size. + +The local `panelState` template accepts `title`, `message` and optional `busy`. +It gives each panel a named section, minimum height and polite status text. +Use plain messages without response bodies or stack traces. Active units, +previous units and recommended order have separate busy/error state. Failed +refreshes retain the last successful data; initial empty results get explicit +messages. Refresh buttons remain mounted and keyboard-focusable while busy, +use `aria-disabled`, and guard repeat activation in their handlers. + +Active projects initially come from the application's global loading contract. +The dashboard does not replace the app's sign-in/bootstrap error handling. +Previous projects use a separate cache so loading history cannot contaminate +the active project cache. All component-owned requests stop on destruction. + +Filter/sort menus use native buttons with checked states. The result count is +announced in one polite live region; input focus remains on the control that +changed the results. Phone disclosures retain their toggle and `aria-expanded`. +Count badges use the existing `#3939ff` token with white text (6.56:1 contrast), +and explicitly name the count and task. Unread comments are not exposed as an +unexplained digit. + +## Validation + +Use synthetic projects for browser checks at 320, 390, 768, 1024 and 1440 px, +in both density modes, with long names and enough tasks to scroll. Exercise +available/no/unavailable feedback, no matches, empty units, previous-unit +failure/retry, and a failed refresh with existing data. Tab through every +control; open filters with the keyboard; verify focus survives refresh. At +200% zoom check wrapping and the continued availability of the project link. + +The focused component tests cover combined filters, missing metadata, actual +task/feedback hrefs, stale data and recovery, duplicate refresh suppression, +focus retention, counts and density. Route tests cover student access and +scope restoration. Browser, test and build results for this change are recorded +in the [validation record](evidence/cross-project-ui/README.md) and PR. Real screen-reader sign-off remains +a reviewer activity; automated accessible-name assertions do not claim it. diff --git a/docs/cross-unit-dashboard.md b/docs/cross-unit-dashboard.md new file mode 100644 index 0000000000..9648e4f317 --- /dev/null +++ b/docs/cross-unit-dashboard.md @@ -0,0 +1,36 @@ +# Cross-project dashboard + +The student-only `/dashboard` route shows authorised tasks across active and +previous units. It supports combined unit/task search, status, grade, date and +staff-feedback filters, task ordering, and direct project/task/feedback links. + +The dashboard was ported from the old 10.x cross-unit branch onto 11.0.x and +translated from UI-Router to Angular Router. It is now implemented and tested +on `11.0.x`; the original root-level skeleton status is retired. + +## Sources and verification + +- `src/app/dashboard/f-cross-dashboard.component.{ts,html,scss}` owns mapping, + filters, grid, density and panel states. +- `src/app/dashboard/list-item/` owns task rows and expanded actions. +- `src/app/app.routes.ts` owns student route access and feedback destinations. +- `src/app/projects/states/index/global-state.service.ts` loads active projects; + `ProjectService` requests previous units with task definitions in a separate cache. +- The API's `GET /projects` scopes project/task metadata to the authenticated user. + +Run `npm run test:ci -- --include='src/app/dashboard/**/*.spec.ts'` using the +repository's Angular Vitest builder. See [UI handover](cross-project-ui.md) for +current validation results, state behaviour and feedback semantics. Review +[privacy-safe evidence guidance](CPD-Q06-privacy-safe-screenshot-checklist.md) +before capturing examples. + +## Sample data + +These values are entirely synthetic and are intended only as a visual reference for dashboard +mockups. They are not an API schema or an automated-test fixture. Dates use `DD/MM/YYYY`. + +| Unit code | Example task due date | Unit completion | Example task status | +| --------- | --------------------- | --------------- | ------------------- | +| COS10001 | 17/10/2026 | 10% | `not_started` | +| COS20007 | 19/08/2026 | 100% | `complete` | +| COS30046 | 25/08/2026 | 90% | `working_on_it` | diff --git a/src/app/account/edit-profile/edit-profile.component.html b/src/app/account/edit-profile/edit-profile.component.html index 43c2d6788c..6fb40b647e 100644 --- a/src/app/account/edit-profile/edit-profile.component.html +++ b/src/app/account/edit-profile/edit-profile.component.html @@ -1,9 +1,13 @@ -
-
-
+
+
+
@if (!loading) { }
-
+
diff --git a/src/app/account/edit-profile/edit-profile.component.scss b/src/app/account/edit-profile/edit-profile.component.scss index 627d96752e..d5fb4d1e9d 100644 --- a/src/app/account/edit-profile/edit-profile.component.scss +++ b/src/app/account/edit-profile/edit-profile.component.scss @@ -3,5 +3,8 @@ } .form-container { + min-width: 0; + width: 100%; max-width: 800px; + padding-inline: 1rem; } diff --git a/src/app/admin/institution-settings/activity-type-list/activity-type-list.component.html b/src/app/admin/institution-settings/activity-type-list/activity-type-list.component.html index 7d53f9eb83..1647f92652 100644 --- a/src/app/admin/institution-settings/activity-type-list/activity-type-list.component.html +++ b/src/app/admin/institution-settings/activity-type-list/activity-type-list.component.html @@ -7,30 +7,18 @@

Activities

@if (loadingActivities) {
-
- @for (width of ['30%', '22%', '4%']; track $index) { - - } -
- - @for (row of skeletonRows; track row) { -
- - - -
- } + +
} diff --git a/src/app/admin/institution-settings/activity-type-list/activity-type-list.component.ts b/src/app/admin/institution-settings/activity-type-list/activity-type-list.component.ts index 87f9b7e71c..f2fc05fa45 100644 --- a/src/app/admin/institution-settings/activity-type-list/activity-type-list.component.ts +++ b/src/app/admin/institution-settings/activity-type-list/activity-type-list.component.ts @@ -5,6 +5,7 @@ import {MatTable, MatTableDataSource} from '@angular/material/table'; import {finalize} from 'rxjs'; import {ActivityType, ActivityTypeService} from 'src/app/api/models/doubtfire-model'; import {EntityFormComponent} from 'src/app/common/entity-form/entity-form.component'; +import {ConfirmationModalService} from 'src/app/common/modals/confirmation-modal/confirmation-modal.service'; import {AlertService} from 'src/app/common/services/alert.service'; @Component({ @@ -26,13 +27,13 @@ export class ActivityTypeListComponent activityTypes: ActivityType[] = new Array(); dataSource = new MatTableDataSource(this.activityTypes); loadingActivities = true; - skeletonRows = Array.from({length: 3}, (_, index) => index); // Calls the parent's constructor, passing in an object // that maps all of the form controls that this form consists of. constructor( private activityTypeService: ActivityTypeService, private alertService: AlertService, + private confirmationModal: ConfirmationModalService, ) { super( { @@ -84,14 +85,22 @@ export class ActivityTypeListComponent } deleteActivity(activity: ActivityType) { - this.delete(activity, this.activityTypes, this.activityTypeService).subscribe({ - next: () => { - this.alertService.success(`${activity.name} has been deleted.`, 2000); + this.confirmationModal.show( + `Delete activity type ${activity.name}`, + 'This activity type is used by tutorial streams across every unit. Deleting it cannot be undone.', + () => { + this.delete(activity, this.activityTypes, this.activityTypeService).subscribe({ + next: () => { + this.alertService.success(`${activity.name} has been deleted.`, 2000); + }, + error: (response) => { + this.alertService.error(response.error?.error || 'Unable to delete activity type.'); + }, + }); }, - error: (response) => { - this.alertService.error(response.error?.error || 'Unable to delete activity type.'); - }, - }); + undefined, + 'Delete', + ); } // Sorting function to sort data when sort diff --git a/src/app/admin/institution-settings/campuses/campus-list/campus-list.component.ts b/src/app/admin/institution-settings/campuses/campus-list/campus-list.component.ts index b953bc3f16..ed5b1b9852 100644 --- a/src/app/admin/institution-settings/campuses/campus-list/campus-list.component.ts +++ b/src/app/admin/institution-settings/campuses/campus-list/campus-list.component.ts @@ -4,6 +4,7 @@ import {MatSort, Sort} from '@angular/material/sort'; import {MatTable, MatTableDataSource} from '@angular/material/table'; import {Campus, CampusService} from 'src/app/api/models/doubtfire-model'; import {EntityFormComponent} from 'src/app/common/entity-form/entity-form.component'; +import {ConfirmationModalService} from 'src/app/common/modals/confirmation-modal/confirmation-modal.service'; import {AlertService} from 'src/app/common/services/alert.service'; @Component({ @@ -29,6 +30,7 @@ export class CampusListComponent extends EntityFormComponent implements constructor( private campusService: CampusService, private alerts: AlertService, + private confirmationModal: ConfirmationModalService, ) { super( { @@ -80,14 +82,22 @@ export class CampusListComponent extends EntityFormComponent implements // This method is called when the delete button is clicked deleteCampus(campus: Campus) { - this.delete(campus, this.campuses, this.campusService).subscribe({ - next: () => { - this.alerts.success(`${campus.name} has been deleted.`, 2000); + this.confirmationModal.show( + `Delete campus ${campus.name}`, + 'This campus is used by tutorials and students across every unit. Deleting it cannot be undone.', + () => { + this.delete(campus, this.campuses, this.campusService).subscribe({ + next: () => { + this.alerts.success(`${campus.name} has been deleted.`, 2000); + }, + error: (response) => { + this.alerts.error(response.error?.error || 'Unable to delete campus.'); + }, + }); }, - error: (response) => { - this.alerts.error(response.error?.error || 'Unable to delete campus.'); - }, - }); + undefined, + 'Delete', + ); } // Sorting function to sort data when sort diff --git a/src/app/admin/institution-settings/institution-settings.component.html b/src/app/admin/institution-settings/institution-settings.component.html index e57317aca4..b10bafbc64 100644 --- a/src/app/admin/institution-settings/institution-settings.component.html +++ b/src/app/admin/institution-settings/institution-settings.component.html @@ -1,4 +1,4 @@ -
+
@switch (currentTab.routeSegment) { @case ('campuses') { - + + + } @case ('activities') { - + + + } @case ('teaching-periods') { - + + + } @case ('learning-outcomes') { -
-
-
-

Learning Outcomes

-

Manage global learning outcomes for the institution

+ +
+
+
+

Learning Outcomes

+

Manage global learning outcomes for the institution

+
+
- -
+ } @case ('overseer-images') { - + + + } @case ('turnitin') { - +
+ +
} }
diff --git a/src/app/admin/institution-settings/institution-settings.component.scss b/src/app/admin/institution-settings/institution-settings.component.scss index 7017d84036..b1baa5aebe 100644 --- a/src/app/admin/institution-settings/institution-settings.component.scss +++ b/src/app/admin/institution-settings/institution-settings.component.scss @@ -1,6 +1,3 @@ -.institution-settings-container { - padding: 0 5rem 0 5rem; -} .learning-outcomes-container { padding: 0 1rem 1rem 1rem; } diff --git a/src/app/admin/institution-settings/overseer-images/overseer-image-list.component.html b/src/app/admin/institution-settings/overseer-images/overseer-image-list.component.html index c9b0d15df4..0a8fc23f53 100644 --- a/src/app/admin/institution-settings/overseer-images/overseer-image-list.component.html +++ b/src/app/admin/institution-settings/overseer-images/overseer-image-list.component.html @@ -18,38 +18,18 @@

@if (loadingImages) {
-
- @for (width of ['22%', '18%', '8%', '16%', '10%', '4%']; track $index) { - - } -
- - @for (row of skeletonRows; track row) { -
- - - - - -
- } + +
} diff --git a/src/app/admin/institution-settings/overseer-images/overseer-image-list.component.ts b/src/app/admin/institution-settings/overseer-images/overseer-image-list.component.ts index 9f7ab13091..52d9476030 100644 --- a/src/app/admin/institution-settings/overseer-images/overseer-image-list.component.ts +++ b/src/app/admin/institution-settings/overseer-images/overseer-image-list.component.ts @@ -39,7 +39,6 @@ export class OverseerImageListComponent dataSource = new MatTableDataSource(this.overseerImages); loading = false; loadingImages = true; - skeletonRows = Array.from({length: 2}, (_, index) => index); public diskSpace: number | null = null; diff --git a/src/app/admin/states/teaching-periods/teaching-period-list/teaching-period-list.component.spec.ts b/src/app/admin/states/teaching-periods/teaching-period-list/teaching-period-list.component.spec.ts index 6b38a84b69..c589b103d8 100644 --- a/src/app/admin/states/teaching-periods/teaching-period-list/teaching-period-list.component.spec.ts +++ b/src/app/admin/states/teaching-periods/teaching-period-list/teaching-period-list.component.spec.ts @@ -1,10 +1,14 @@ -import {beforeEach, describe, expect, it} from 'vitest'; +import {beforeEach, describe, expect, it, vi} from 'vitest'; import {NO_ERRORS_SCHEMA} from '@angular/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; import {MatDialog} from '@angular/material/dialog'; +import {of} from 'rxjs'; import {TeachingPeriodService} from 'src/app/api/services/teaching-period.service'; import {TeachingPeriodUnitImportService} from '../teaching-period-unit-import/teaching-period-unit-import.dialog'; -import {TeachingPeriodListComponent} from './teaching-period-list.component'; +import { + NewTeachingPeriodDialogComponent, + TeachingPeriodListComponent, +} from './teaching-period-list.component'; const emptyProvider = {}; @@ -35,3 +39,20 @@ describe('TeachingPeriodListComponent', () => { expect(component).toBeTruthy(); }); }); + +describe('NewTeachingPeriodDialogComponent deleteBreak feedback', () => { + it('confirms to the user when a break is removed', () => { + const alertService = {success: vi.fn(), error: vi.fn()}; + const component = Object.create(NewTeachingPeriodDialogComponent.prototype) as { + alertService: typeof alertService; + deleteBreak(teachingPeriod: unknown, teachingBreak: unknown): void; + }; + component.alertService = alertService; + const teachingPeriod = {removeBreak: vi.fn(() => of({}))}; + + component.deleteBreak(teachingPeriod, {id: 3}); + + expect(teachingPeriod.removeBreak).toHaveBeenCalledWith(3); + expect(alertService.success).toHaveBeenCalledWith('Break removed', 2000); + }); +}); diff --git a/src/app/admin/states/teaching-periods/teaching-period-list/teaching-period-list.component.ts b/src/app/admin/states/teaching-periods/teaching-period-list/teaching-period-list.component.ts index 430dcd9dfd..99dde3f1ff 100644 --- a/src/app/admin/states/teaching-periods/teaching-period-list/teaching-period-list.component.ts +++ b/src/app/admin/states/teaching-periods/teaching-period-list/teaching-period-list.component.ts @@ -119,9 +119,8 @@ export class NewTeachingPeriodDialogComponent { this.newOrSelectedTeachingPeriod .addBreak(this.tempBreak.startDate, this.tempBreak.numberOfWeeks) .subscribe({ - next: (teachingPeriodBreak) => { + next: () => { this.alertService.success('Break added'); - console.log(teachingPeriodBreak); }, error: (response) => { this.alertService.error(`Error adding break. ${response}`); @@ -131,8 +130,8 @@ export class NewTeachingPeriodDialogComponent { deleteBreak(teachingPeriod: TeachingPeriod, teachingBreak: TeachingPeriodBreak): void { teachingPeriod.removeBreak(teachingBreak.id).subscribe({ - next: (teachingPeriodBreak) => { - console.log(teachingPeriodBreak); + next: () => { + this.alertService.success('Break removed', 2000); }, error: (response) => { this.alertService.error(`Error deleting break. ${response}`); diff --git a/src/app/admin/states/users/users.component.ts b/src/app/admin/states/users/users.component.ts index 63a39166bd..ab413adfef 100644 --- a/src/app/admin/states/users/users.component.ts +++ b/src/app/admin/states/users/users.component.ts @@ -86,7 +86,6 @@ export class FUsersComponent implements OnInit, AfterViewInit, OnDestroy { this.fileDownloaderService.downloadFile(this.userService.csvURL, 'Users.csv'); } - // eslint-disable-next-line @typescript-eslint/no-unused-vars // TODO: This needs to be brought out into a service which replaces the old csv-result-modal protected onUserUploadSuccess(event) { const max_full_errors = 5; @@ -108,7 +107,7 @@ export class FUsersComponent implements OnInit, AfterViewInit, OnDestroy { } this.alerts.error(error_string); - this.userService.query(); + this.userService.fetchAll().subscribe({error: (error) => this.alerts.error(error)}); } public showUserModal(user?: User) { diff --git a/src/app/admin/tii-action-log/tii-action-log.component.html b/src/app/admin/tii-action-log/tii-action-log.component.html index 4590ee9977..57506d0170 100644 --- a/src/app/admin/tii-action-log/tii-action-log.component.html +++ b/src/app/admin/tii-action-log/tii-action-log.component.html @@ -27,44 +27,44 @@

Turnitin Actions

{{ action.description }} - + Last Run {{ action.lastRun ? (action.lastRun | date: 'd LLL y') : '' }} - + Retries {{ action.retries }} - + Retry? {{ action.retry }} - + Error Code {{ action.errorCode }} - + Complete? {{ action.complete }} - + Error Message {{ action.errorMessage }} - + @if (!(action.complete || action.retry)) { @@ -87,4 +87,6 @@

Turnitin Actions

[style.background]="selectedTaskDefinition === row ? 'lightblue' : ''" > + +
diff --git a/src/app/admin/tii-action-log/tii-action-log.component.spec.ts b/src/app/admin/tii-action-log/tii-action-log.component.spec.ts index d9cd6e6310..c1a10ce7a8 100644 --- a/src/app/admin/tii-action-log/tii-action-log.component.spec.ts +++ b/src/app/admin/tii-action-log/tii-action-log.component.spec.ts @@ -1,6 +1,10 @@ import {beforeEach, describe, expect, it} from 'vitest'; import {NO_ERRORS_SCHEMA} from '@angular/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {FormsModule} from '@angular/forms'; +import {MatSortModule} from '@angular/material/sort'; +import {MatTableModule} from '@angular/material/table'; +import {from} from 'rxjs'; import {TiiActionService} from 'src/app/api/services/tii-action.service'; import {AlertService} from 'src/app/common/services/alert.service'; import {TiiActionLogComponent} from './tii-action-log.component'; @@ -33,3 +37,58 @@ describe('TiiActionLogComponent', () => { expect(component).toBeTruthy(); }); }); + +describe('TiiActionLogComponent sticky columns (STAFF-15)', () => { + let fixture: ComponentFixture; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + declarations: [TiiActionLogComponent], + imports: [FormsModule, MatSortModule, MatTableModule], + providers: [ + // A Promise-backed observable defers emission to a microtask, same as + // the real HTTP call would - avoids mutating tiiActionsSource inside + // the same synchronous pass ngAfterViewInit runs in. + {provide: TiiActionService, useValue: {query: () => from(Promise.resolve([]))}}, + {provide: AlertService, useValue: emptyProvider}, + ], + schemas: [NO_ERRORS_SCHEMA], + }).compileComponents(); + + fixture = TestBed.createComponent(TiiActionLogComponent); + fixture.detectChanges(); // triggers ngAfterViewInit, which subscribes + await fixture.whenStable(); // let the microtask set tiiActionsSource + fixture.detectChanges(); // render the table against that data source + }); + + // CDK Table applies `position: sticky` via the mat-mdc-table-sticky class (in its + // stylesheet, not inline) and sets the per-instance left/right offset inline. It + // never renders errorCode or complete at all - they're declared in the template + // but absent from the `columns` array, so they don't reach the header row. + function headerCell(column: string): HTMLElement { + return fixture.nativeElement.querySelector(`.mat-column-${column}`); + } + + it('only pins the identifying column (type) to the start', () => { + const type = headerCell('type'); + expect(type.classList.contains('mat-mdc-table-sticky')).toBe(true); + expect(type.style.left).toBe('0px'); + expect(type.style.right).toBe(''); + + for (const column of ['lastRun', 'retries', 'retry', 'errorMessage']) { + const cell = headerCell(column); + expect( + cell.classList.contains('mat-mdc-table-sticky'), + `${column} should not be sticky`, + ).toBe(false); + expect(cell.style.left, `${column} should have no sticky offset`).toBe(''); + } + }); + + it('pins the tools column to the end, not the start, matching the tutorials table', () => { + const tools = headerCell('tiiActionTools'); + expect(tools.classList.contains('mat-mdc-table-sticky')).toBe(true); + expect(tools.style.right).toBe('0px'); + expect(tools.style.left).toBe(''); + }); +}); diff --git a/src/app/admin/tii-action-log/tii-action-log.component.ts b/src/app/admin/tii-action-log/tii-action-log.component.ts index 1f7559a9f9..2f6b29da8b 100644 --- a/src/app/admin/tii-action-log/tii-action-log.component.ts +++ b/src/app/admin/tii-action-log/tii-action-log.component.ts @@ -37,7 +37,6 @@ export class TiiActionLogComponent implements AfterViewInit { ngAfterViewInit(): void { this.tiiActionService.query().subscribe((actions) => { - console.log(actions); this.tiiActionsSource = new MatTableDataSource(actions); this.tiiActionsSource.paginator = this.paginator; this.tiiActionsSource.sort = this.sort; diff --git a/src/app/api/models/course-flow.ts b/src/app/api/models/course-flow.ts new file mode 100644 index 0000000000..63dc9e2d95 --- /dev/null +++ b/src/app/api/models/course-flow.ts @@ -0,0 +1,48 @@ +/** Versioned catalog rules for planning; these do not certify degree eligibility. */ +export interface CourseFlowUnit { + code: string; + name: string; + required: boolean; + prerequisites: string[]; + offered_trimesters: number[]; +} + +export interface CourseFlowCourse { + id: number; + code: string; + name: string; + version: string; + elective_count: number; + units: CourseFlowUnit[]; +} + +export interface CourseFlowPeriod { + year: number; + trimester: number; +} + +export interface CourseFlowSlot extends CourseFlowPeriod { + unit_code: string; + position: number; +} + +export interface CourseFlowIssue { + code: 'missing_required' | 'elective_count' | 'prerequisite' | 'unavailable_trimester'; + message: string; + unit_code?: string; +} + +export interface CourseFlowDraft { + course_id: number; + name: string; + periods: CourseFlowPeriod[]; + slots: CourseFlowSlot[]; +} + +export interface CourseFlowMap extends CourseFlowDraft { + id: number; + lock_version: number; + issues: CourseFlowIssue[]; + complete: boolean; + updated_at: string; +} diff --git a/src/app/api/models/doubtfire-model.ts b/src/app/api/models/doubtfire-model.ts index 60b3bfb8b9..5712349732 100644 --- a/src/app/api/models/doubtfire-model.ts +++ b/src/app/api/models/doubtfire-model.ts @@ -41,6 +41,7 @@ export * from './task-comment/scorm-extension-comment'; export * from './feedback-template'; export * from './communication'; export * from './engagement'; +export * from './course-flow'; // Users -- are students or staff export * from './user/user'; @@ -75,3 +76,7 @@ export * from '../services/communication-condition.service'; export * from '../services/communication-action.service'; export * from '../services/engagement.service'; export * from '../services/engagement-comment.service'; + +// Peer progress -- anonymous cohort progress for a project's target grade +export * from './peer-progress'; +export * from '../services/peer-progress.service'; diff --git a/src/app/api/models/project.ts b/src/app/api/models/project.ts index 48c2720701..d786b963fb 100644 --- a/src/app/api/models/project.ts +++ b/src/app/api/models/project.ts @@ -24,6 +24,11 @@ import {Engagement} from './engagement'; import {StaffNote} from './staff-note'; import {TaskOutcomeAlignment} from './task-outcome-alignment'; +export interface ProjectBurndownSeries { + key: string; + values: [timestamp: number, remaining: number][]; +} + export class Project extends Entity { public id: number; public unit: Unit; @@ -52,7 +57,7 @@ export class Project extends Entity { }[]; public orderScale: number; - public burndownChartData: {key: string; values: number[]}[]; + public burndownChartData: ProjectBurndownSeries[]; public readonly taskCache: EntityCache = new EntityCache(); public readonly staffNoteCache: EntityCache = new EntityCache(); public readonly engagementCache: EntityCache = new EntityCache(); @@ -398,15 +403,15 @@ export class Project extends Entity { } public refreshBurndownChartData(): void { - const result: {key: string; values: number[]}[] = []; + const result: ProjectBurndownSeries[] = []; // Setup the dictionaries to contain the keys and values // key = series name // values = array of [ x, y ] values - const projectedResults = {key: 'Projected', values: []}; - const targetTaskResults = {key: 'Target', values: []}; - const doneTaskResults = {key: 'To Submit', values: []}; - const completeTaskResults = {key: 'To Complete', values: []}; + const projectedResults: ProjectBurndownSeries = {key: 'Projected', values: []}; + const targetTaskResults: ProjectBurndownSeries = {key: 'Target', values: []}; + const doneTaskResults: ProjectBurndownSeries = {key: 'To Submit', values: []}; + const completeTaskResults: ProjectBurndownSeries = {key: 'To Complete', values: []}; result.push(targetTaskResults); result.push(projectedResults); @@ -496,7 +501,7 @@ export class Project extends Entity { dates.forEach((date) => { // get the target values - those from the task definitions // amount remaining is the sum of all tasks due after the date - const targetVal = [ + const targetVal: ProjectBurndownSeries['values'][number] = [ date.getTime(), (targetTasks .filter((taskDef) => taskDef.targetDate >= date) @@ -505,7 +510,7 @@ export class Project extends Entity { ]; // get the done values - those done up to today, or the end of the unit - const doneVal = [ + const doneVal: ProjectBurndownSeries['values'][number] = [ date.getTime(), (total - doneTasks @@ -516,7 +521,7 @@ export class Project extends Entity { ]; // get the completed values - those signed off - const completeVal = [ + const completeVal: ProjectBurndownSeries['values'][number] = [ date.getTime(), (total - completedTasks @@ -527,7 +532,10 @@ export class Project extends Entity { ]; // projected value is based on amount done - const projectedVal = [date.getTime(), projectedRemaining / total]; + const projectedVal: ProjectBurndownSeries['values'][number] = [ + date.getTime(), + projectedRemaining / total, + ]; // add one week's worth of completion data projectedRemaining -= completionRate; diff --git a/src/app/api/models/task.ts b/src/app/api/models/task.ts index bac5be4451..476eba576d 100644 --- a/src/app/api/models/task.ts +++ b/src/app/api/models/task.ts @@ -50,6 +50,24 @@ export class Task extends Entity { status: TaskStatusEnum = 'not_started'; dueDate: Date; + effectiveDeadline?: Date; + effectiveDeadlineDate?: Date; + effectiveDeadlineReason?: string; + effectiveDeadlineSourceId?: number; + + public get effectiveDeadlineExplanation(): string { + switch (this.effectiveDeadlineReason) { + case 'post_feedback_extension': + return 'Extended after feedback requiring further action.'; + case 'approved_extension': + return 'Includes an approved extension.'; + case 'flexible_date': + return 'Your planned submission date.'; + default: + return 'The standard due date for this task.'; + } + } + extensions: number; scormExtensions: number; submissionDate: Date; @@ -60,6 +78,7 @@ export class Task extends Entity { includeInPortfolio: boolean = true; similarityFlag: boolean = false; numNewComments: number = 0; + hasFeedback?: boolean; hasExtensions: boolean; moderationType: 'random_sample' | 'escalation' | 'first_feedback'; @@ -905,7 +924,11 @@ export class Task extends Entity { ); } - public processTaskStatusChange(expectedStatus: TaskStatusEnum, alerts: AlertService) { + public processTaskStatusChange( + expectedStatus: TaskStatusEnum, + alerts: AlertService, + submissionCompleted: boolean = false, + ) { if (this.inTimeExceeded() && !this.isPastDeadline()) { alerts.message( 'You have submitted after the deadline for feedback. Your task will not be reviewed by a tutor. It is now your responsibility to ensure this task meets the required standard.', @@ -920,7 +943,7 @@ export class Task extends Entity { } this.getSubmissionDetails().subscribe(); const taskService: TaskService = AppInjector.get(TaskService); - taskService.notifyStatusChange(this); + taskService.notifyTransitionComplete(this, submissionCompleted); } public async markAsDiscussed(reasonText?: string) { @@ -994,6 +1017,7 @@ export class Task extends Entity { status: TaskStatusEnum, markAsDiscussed?: boolean, triggerRecursiveFix?: boolean, + submissionCompleted: boolean = false, ) { const oldStatus = this.status; const oldGrade = this.grade; @@ -1043,7 +1067,7 @@ export class Task extends Entity { this.project.taskCache.delete(this.definition.abbreviation); this.project.taskCache.add(this); } - this.processTaskStatusChange(status, alerts); + this.processTaskStatusChange(status, alerts, submissionCompleted); taskService.notifyStatusChange(this); }, error: (error) => { @@ -1093,7 +1117,12 @@ export class Task extends Entity { } else if (requiresFileUpload && !this.isReadyForUpload) { alerts.error('Complete Knowledge Check first to submit files', 6000); } else { - await this.updateTaskStatus(status); + await this.updateTaskStatus( + status, + undefined, + undefined, + TaskStatus.SUBMITTABLE_STATUSES.includes(status), + ); } } diff --git a/src/app/api/models/unit-role.ts b/src/app/api/models/unit-role.ts index 1436c96847..f3d138ec24 100644 --- a/src/app/api/models/unit-role.ts +++ b/src/app/api/models/unit-role.ts @@ -13,7 +13,7 @@ export class UnitRole extends Entity { unit: Unit; observerOnly: boolean; canMarkOverflowTasks: boolean; - mentorId: number; + mentorId: number | null; tutorNoteCount: number; public get mentor(): UnitRole { diff --git a/src/app/api/models/unit.spec.ts b/src/app/api/models/unit.spec.ts new file mode 100644 index 0000000000..8222062ca4 --- /dev/null +++ b/src/app/api/models/unit.spec.ts @@ -0,0 +1,59 @@ +import {describe, expect, it, vi} from 'vitest'; +import {Project} from './project'; +import {Unit} from './unit'; + +describe('Unit.findStudent', () => { + it('returns the same project instance as studentCache.get', () => { + const unit = new Unit(); + const project = new Project(unit); + + project.id = 42; + unit.studentCache.add(project); + + expect(unit.findStudent(42)).toBe(unit.studentCache.get(42)); + }); + + it('does not scan the students array', () => { + const unit = new Unit(); + const project = new Project(unit); + + project.id = 42; + unit.studentCache.add(project); + + vi.spyOn(unit, 'students', 'get').mockImplementation(() => { + throw new Error('students array was scanned'); + }); + + expect(unit.findStudent(42)).toBe(project); + }); + + it('returns undefined when the student does not exist', () => { + const unit = new Unit(); + + expect(unit.findStudent(999)).toBeUndefined(); + }); +}); + +describe('Unit.findStudent (via stubbed cache)', () => { + // Build a Unit without running the constructor so the test stays free of the + // Angular injector, then give it a stub studentCache. + function unitWithCache(entries: Record): Unit { + const unit = Object.create(Unit.prototype) as Unit; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (unit as any).studentCache = { + get: (key: number) => entries[key], + }; + return unit; + } + + it('returns the same instance the cache holds for that id', () => { + const project = {id: 7} as unknown; + const unit = unitWithCache({7: project}); + expect(unit.findStudent(7)).toBe(project); + }); + + it('returns undefined when no project has that id', () => { + const unit = unitWithCache({7: {id: 7}}); + expect(unit.findStudent(99)).toBeUndefined(); + }); +}); diff --git a/src/app/api/models/unit.ts b/src/app/api/models/unit.ts index 6c5b87dcb7..f82998c78d 100644 --- a/src/app/api/models/unit.ts +++ b/src/app/api/models/unit.ts @@ -195,7 +195,7 @@ export class Unit extends Entity { } public findStudent(id: number): Project { - return this.students.find((s) => s.id === id); + return this.studentCache.get(id); } public studentEnrolled(id: number): boolean { diff --git a/src/app/api/models/user/user.ts b/src/app/api/models/user/user.ts index 5a98e22551..23272b320c 100644 --- a/src/app/api/models/user/user.ts +++ b/src/app/api/models/user/user.ts @@ -15,10 +15,14 @@ export class User extends Entity { public email: string; public username: string; public nickname: string; + public displayName: string; public systemRole: 'Admin' | 'Convenor' | 'Tutor' | 'Student' | 'Auditor'; public receiveTaskNotifications: boolean; public receivePortfolioNotifications: boolean; public receiveFeedbackNotifications: boolean; + public displayPeerProgress: boolean; + public themePreference: 'light' | 'dark' | 'system' | null; + public themePreferenceUpdatedAt: string | null; public hasRunFirstTimeSetup: boolean; public authenticationToken: string; public authenticationTokenExpiry: string; @@ -65,7 +69,7 @@ export class User extends Entity { public matches(text: string): boolean { return ( this.studentId?.toLowerCase().indexOf(text) >= 0 || - this.name.toLowerCase().indexOf(text) >= 0 || + this.displayName?.toLowerCase().indexOf(text) >= 0 || this.firstName.toLowerCase().indexOf(text) >= 0 || this.lastName.toLowerCase().indexOf(text) >= 0 || this.email.toLowerCase().indexOf(text) >= 0 || diff --git a/src/app/api/services/course-flow.service.spec.ts b/src/app/api/services/course-flow.service.spec.ts new file mode 100644 index 0000000000..60533fe805 --- /dev/null +++ b/src/app/api/services/course-flow.service.spec.ts @@ -0,0 +1,210 @@ +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; +import { + HTTP_INTERCEPTORS, + HttpClient, + HttpErrorResponse, + provideHttpClient, + withInterceptorsFromDi, +} from '@angular/common/http'; +import {HttpTestingController, provideHttpClientTesting} from '@angular/common/http/testing'; +import {TestBed} from '@angular/core/testing'; +import {HttpAuthenticationInterceptor} from 'src/app/common/services/http-authentication.interceptor'; +import {PRESERVE_HTTP_ERROR_RESPONSE} from 'src/app/common/services/http-error-context'; +import {HttpErrorInterceptor} from 'src/app/common/services/http-error.interceptor'; +import API_URL from 'src/app/config/constants/apiUrl'; +import {CourseFlowDraft} from '../models/course-flow'; +import {AuthenticationService} from './authentication.service'; +import {CourseFlowService} from './course-flow.service'; +import {UserService} from './user.service'; + +const draft: CourseFlowDraft = { + course_id: 7, + name: 'Personal study plan', + periods: [ + {year: 2026, trimester: 1}, + {year: 2027, trimester: 2}, + ], + slots: [{unit_code: 'REQ1', year: 2026, trimester: 1, position: 3}], +}; + +describe('Course Flow authenticated API contract', () => { + let service: CourseFlowService; + let http: HttpTestingController; + let currentUser: { + authenticationToken: string; + authenticationTokenExpiry: string | null; + username: string; + }; + let authentication: { + attemptLoginUsingRefreshToken: ReturnType< + typeof vi.fn<(callback: (success: boolean) => void) => void> + >; + timeoutAuthentication: ReturnType; + }; + const base = `${API_URL}/courseflow`; + + beforeEach(() => { + currentUser = { + authenticationToken: 'session-token', + authenticationTokenExpiry: null, + username: 'student', + }; + authentication = { + attemptLoginUsingRefreshToken: vi.fn((callback: (success: boolean) => void) => { + currentUser.authenticationToken = 'renewed-token'; + callback(true); + }), + timeoutAuthentication: vi.fn(), + }; + vi.spyOn(console, 'error').mockImplementation(() => {}); + TestBed.configureTestingModule({ + providers: [ + provideHttpClient(withInterceptorsFromDi()), + provideHttpClientTesting(), + {provide: UserService, useValue: {currentUser, isAnonymousUser: () => false}}, + {provide: AuthenticationService, useValue: authentication}, + {provide: HTTP_INTERCEPTORS, useClass: HttpAuthenticationInterceptor, multi: true}, + {provide: HTTP_INTERCEPTORS, useClass: HttpErrorInterceptor, multi: true}, + ], + }); + service = TestBed.inject(CourseFlowService); + http = TestBed.inject(HttpTestingController); + }); + afterEach(() => { + http.verify(); + vi.restoreAllMocks(); + }); + + it('reads the student-accessible catalog and owner-scoped plans without teaching-unit or user-ID requests', () => { + service.getCourses().subscribe(); + const catalog = http.expectOne({method: 'GET', url: `${base}/courses`}); + expect(catalog.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(true); + expect(catalog.request.headers.get('Auth-Token')).toBe('session-token'); + expect(catalog.request.headers.get('Username')).toBe('student'); + catalog.flush([]); + service.getMaps().subscribe(); + const maps = http.expectOne({method: 'GET', url: `${base}/maps`}); + expect(maps.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(true); + maps.flush([]); + service.getMap(12).subscribe(); + const map = http.expectOne({method: 'GET', url: `${base}/maps/12`}); + expect(map.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(true); + map.flush({id: 12}); + http.expectNone((request) => request.url.includes('/api/units')); + }); + + it('sends a complete atomic draft body including empty periods and unit codes', () => { + service.createMap(draft).subscribe(); + const request = http.expectOne({method: 'POST', url: `${base}/maps`}); + expect(request.request.body).toEqual(draft); + expect(request.request.body.user_id).toBeUndefined(); + expect(request.request.body.params).toBeUndefined(); + expect(request.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(true); + request.flush({...draft, id: 12, lock_version: 0}); + }); + + it('sends the expected lock version on updates and preserves server conflicts for the caller', () => { + let status: number; + service.updateMap(12, draft, 4).subscribe({error: (error) => (status = error.status)}); + const request = http.expectOne({method: 'PUT', url: `${base}/maps/12`}); + expect(request.request.body).toEqual({...draft, lock_version: 4}); + expect(request.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(true); + request.flush({error: 'Conflict'}, {status: 409, statusText: 'Conflict'}); + expect(status).toBe(409); + }); + + it('sends a real query lock version on deletion', () => { + service.deleteMap(12, 4).subscribe(); + const request = http.expectOne({method: 'DELETE', url: `${base}/maps/12?lock_version=4`}); + expect(request.request.body).toBeNull(); + expect(request.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(true); + request.flush(null, {status: 204, statusText: 'No Content'}); + }); + + it('preserves owner-scoped 404 responses through the real interceptor', () => { + let failure: HttpErrorResponse; + service.getMap(99).subscribe({error: (error) => (failure = error)}); + http + .expectOne(`${base}/maps/99`) + .flush({error: 'Plan not found'}, {status: 404, statusText: 'Not Found'}); + expect(failure).toBeInstanceOf(HttpErrorResponse); + expect(failure.status).toBe(404); + expect(failure.error).toEqual({error: 'Plan not found'}); + expect(authentication.attemptLoginUsingRefreshToken).not.toHaveBeenCalled(); + }); + + it('preserves conflicts after refreshing credentials without bypassing authentication', () => { + let failure: HttpErrorResponse; + service.updateMap(12, draft, 4).subscribe({error: (error) => (failure = error)}); + http.expectOne(`${base}/maps/12`).flush({}, {status: 419, statusText: 'Expired'}); + const retry = http.expectOne(`${base}/maps/12`); + expect(retry.request.headers.get('Auth-Token')).toBe('renewed-token'); + expect(retry.request.headers.get('Username')).toBe('student'); + expect(retry.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(true); + expect(retry.request.body).toEqual({...draft, lock_version: 4}); + retry.flush({error: 'Conflict'}, {status: 409, statusText: 'Conflict'}); + expect(failure).toBeInstanceOf(HttpErrorResponse); + expect(failure.status).toBe(409); + expect(authentication.attemptLoginUsingRefreshToken).toHaveBeenCalledOnce(); + expect(authentication.timeoutAuthentication).not.toHaveBeenCalled(); + }); + + it('preserves errors for requests queued behind the same token refresh', () => { + let completeRefresh: (success: boolean) => void; + authentication.attemptLoginUsingRefreshToken.mockImplementation( + (callback) => (completeRefresh = callback), + ); + let firstFailure: HttpErrorResponse; + let queuedFailure: HttpErrorResponse; + service.getMap(12).subscribe({error: (error) => (firstFailure = error)}); + service.getMap(13).subscribe({error: (error) => (queuedFailure = error)}); + http.expectOne(`${base}/maps/12`).flush({}, {status: 419, statusText: 'Expired'}); + http.expectOne(`${base}/maps/13`).flush({}, {status: 419, statusText: 'Expired'}); + expect(authentication.attemptLoginUsingRefreshToken).toHaveBeenCalledOnce(); + currentUser.authenticationToken = 'shared-renewed-token'; + completeRefresh(true); + const firstRetry = http.expectOne(`${base}/maps/12`); + const queuedRetry = http.expectOne(`${base}/maps/13`); + expect(queuedRetry.request.headers.get('Auth-Token')).toBe('shared-renewed-token'); + expect(queuedRetry.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(true); + firstRetry.flush({}, {status: 404, statusText: 'Not Found'}); + queuedRetry.flush({}, {status: 409, statusText: 'Conflict'}); + expect(firstFailure.status).toBe(404); + expect(queuedFailure).toBeInstanceOf(HttpErrorResponse); + expect(queuedFailure.status).toBe(409); + }); + + it('keeps legacy message errors for callers that do not opt in', () => { + let failure: unknown; + TestBed.inject(HttpClient) + .get(`${API_URL}/units`) + .subscribe({error: (error) => (failure = error)}); + const request = http.expectOne(`${API_URL}/units`); + expect(request.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(false); + request.flush({error: 'Unit not found'}, {status: 404, statusText: 'Not Found'}); + expect(failure).toBe('Unit not found'); + }); + + it('still expires the session when refreshing credentials fails', () => { + authentication.attemptLoginUsingRefreshToken.mockImplementation((callback) => callback(false)); + let failure: Error; + service.getMaps().subscribe({error: (error) => (failure = error)}); + http.expectOne(`${base}/maps`).flush({}, {status: 419, statusText: 'Expired'}); + expect(authentication.timeoutAuthentication).toHaveBeenCalledOnce(); + expect(failure).toBeInstanceOf(Error); + expect(failure.message).toBe('Authentication timed out'); + }); + + it('keeps legacy message errors after token refresh too', () => { + let failure: unknown; + TestBed.inject(HttpClient) + .get(`${API_URL}/units`) + .subscribe({error: (error) => (failure = error)}); + http.expectOne(`${API_URL}/units`).flush({}, {status: 419, statusText: 'Expired'}); + const retry = http.expectOne(`${API_URL}/units`); + expect(retry.request.headers.get('Auth-Token')).toBe('renewed-token'); + expect(retry.request.context.get(PRESERVE_HTTP_ERROR_RESPONSE)).toBe(false); + retry.flush({error: 'Unit not found'}, {status: 404, statusText: 'Not Found'}); + expect(failure).toBe('Unit not found'); + }); +}); diff --git a/src/app/api/services/course-flow.service.ts b/src/app/api/services/course-flow.service.ts new file mode 100644 index 0000000000..54fa5bcbf8 --- /dev/null +++ b/src/app/api/services/course-flow.service.ts @@ -0,0 +1,56 @@ +import {HttpClient, HttpContext} from '@angular/common/http'; +import {Injectable, inject} from '@angular/core'; +import {Observable} from 'rxjs'; +import {PRESERVE_HTTP_ERROR_RESPONSE} from 'src/app/common/services/http-error-context'; +import API_URL from 'src/app/config/constants/apiUrl'; +import {CourseFlowCourse, CourseFlowDraft, CourseFlowMap} from '../models/course-flow'; + +@Injectable({providedIn: 'root'}) +export class CourseFlowService { + private readonly http = inject(HttpClient); + private readonly baseUrl = `${API_URL}/courseflow`; + + getCourses(): Observable { + return this.http.get(`${this.baseUrl}/courses`, { + context: this.errorContext(), + }); + } + + getMaps(): Observable { + return this.http.get(`${this.baseUrl}/maps`, {context: this.errorContext()}); + } + + getMap(id: number): Observable { + return this.http.get(`${this.baseUrl}/maps/${id}`, { + context: this.errorContext(), + }); + } + + createMap(draft: CourseFlowDraft): Observable { + return this.http.post(`${this.baseUrl}/maps`, draft, { + context: this.errorContext(), + }); + } + + updateMap(id: number, draft: CourseFlowDraft, lockVersion: number): Observable { + return this.http.put( + `${this.baseUrl}/maps/${id}`, + { + ...draft, + lock_version: lockVersion, + }, + {context: this.errorContext()}, + ); + } + + deleteMap(id: number, lockVersion: number): Observable { + return this.http.delete(`${this.baseUrl}/maps/${id}`, { + params: {lock_version: lockVersion}, + context: this.errorContext(), + }); + } + + private errorContext(): HttpContext { + return new HttpContext().set(PRESERVE_HTTP_ERROR_RESPONSE, true); + } +} diff --git a/src/app/api/services/spec/task.service.spec.ts b/src/app/api/services/spec/task.service.spec.ts new file mode 100644 index 0000000000..81a13dba8f --- /dev/null +++ b/src/app/api/services/spec/task.service.spec.ts @@ -0,0 +1,109 @@ +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; +import {provideHttpClient} from '@angular/common/http'; +import {HttpTestingController, provideHttpClientTesting} from '@angular/common/http/testing'; +import {TestBed} from '@angular/core/testing'; +import {Unit} from 'src/app/api/models/doubtfire-model'; +import {Task} from 'src/app/api/models/task'; +import {TaskService} from '../task.service'; + +describe('TaskService', () => { + let service: TaskService; + let httpMock: HttpTestingController; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [TaskService, provideHttpClient(), provideHttpClientTesting()], + }); + + service = TestBed.inject(TaskService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpMock.verify(); + }); + + it('maps has_feedback from the API to Task.hasFeedback', () => { + const project = { + id: 2, + unit: undefined, + }; + + const unit = { + id: 1, + findStudent: vi.fn(() => project), + taskDef: vi.fn((id: number) => ({id})), + incorporateTasks: vi.fn(), + } as unknown as Unit; + + project.unit = unit; + + let result: Task[]; + + service.queryTasksForTaskInbox(unit).subscribe((tasks) => { + result = tasks; + }); + + const request = httpMock.expectOne( + (req) => req.url.endsWith('/units/1/tasks/inbox') && req.method === 'GET', + ); + + request.flush([ + { + id: 27, + project_id: 2, + task_definition_id: 1, + has_feedback: true, + }, + ]); + + expect(result).toHaveLength(1); + expect(result[0].hasFeedback).toBe(true); + }); + + it('keeps general status changes separate from completed submissions', () => { + const task = {} as Task; + const statusUpdated = vi.fn(); + const submissionCompleted = vi.fn(); + + service.taskStatusUpdated$.subscribe(statusUpdated); + service.taskSubmissionCompleted$.subscribe(submissionCompleted); + + service.notifyStatusChange(task); + + expect(statusUpdated).toHaveBeenCalledOnce(); + expect(statusUpdated).toHaveBeenCalledWith(task); + expect(submissionCompleted).not.toHaveBeenCalled(); + }); + + it('broadcasts both events when a submission completes', () => { + const task = {} as Task; + const statusUpdated = vi.fn(); + const submissionCompleted = vi.fn(); + + service.taskStatusUpdated$.subscribe(statusUpdated); + service.taskSubmissionCompleted$.subscribe(submissionCompleted); + + service.notifyTransitionComplete(task, true); + + expect(statusUpdated).toHaveBeenCalledOnce(); + expect(statusUpdated).toHaveBeenCalledWith(task); + expect(submissionCompleted).toHaveBeenCalledOnce(); + expect(submissionCompleted).toHaveBeenCalledWith(task); + }); + + it('does not treat a non-submission transition as a completed submission', () => { + const task = {} as Task; + const statusUpdated = vi.fn(); + const submissionCompleted = vi.fn(); + + service.taskStatusUpdated$.subscribe(statusUpdated); + service.taskSubmissionCompleted$.subscribe(submissionCompleted); + + service.notifyTransitionComplete(task, false); + + expect(statusUpdated).toHaveBeenCalledOnce(); + expect(statusUpdated).toHaveBeenCalledWith(task); + expect(submissionCompleted).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/services/spec/user.service.spec.ts b/src/app/api/services/spec/user.service.spec.ts index 2e03e7e9c4..ff59c9bdf0 100644 --- a/src/app/api/services/spec/user.service.spec.ts +++ b/src/app/api/services/spec/user.service.spec.ts @@ -53,6 +53,7 @@ describe('UserService', () => { firstName: 'Jake', lastName: 'renzella', email: 'jake@jake.jake', + displayPeerProgress: false, }); }); @@ -74,6 +75,41 @@ describe('UserService', () => { receive_portfolio_notifications: false, receive_feedback_notifications: false, receive_task_notifications: false, + display_peer_progress: false, + }); + }); + + it('serialises the peer progress display preference when updating a profile', () => { + const user = new User(); + user.id = 1; + user.displayPeerProgress = false; + + userService.update(user).subscribe(); + + const req = httpMock.expectOne('http://localhost:3000/api/users/1'); + expect(req.request.method).toBe('PUT'); + expect(req.request.body).toMatchObject({ + user: {display_peer_progress: false}, + }); + req.flush({id: 1, display_peer_progress: false}); + }); + + it('updates only the account theme preference and maps its server timestamp', () => { + const user = new User(); + user.id = 1; + + userService.updateThemePreference(user, 'dark').subscribe((updatedUser) => { + expect(updatedUser.themePreference).toBe('dark'); + expect(updatedUser.themePreferenceUpdatedAt).toBe('2026-09-01T01:02:03Z'); + }); + + const req = httpMock.expectOne('http://localhost:3000/api/users/1'); + expect(req.request.method).toBe('PUT'); + expect(req.request.body).toEqual({user: {theme_preference: 'dark'}}); + req.flush({ + id: 1, + theme_preference: 'dark', + theme_preference_updated_at: '2026-09-01T01:02:03Z', }); }); @@ -157,127 +193,109 @@ describe('UserService', () => { u.receiveFeedbackNotifications = false; u.receiveTaskNotifications = false; - userService.update(u).subscribe( - (result) => { - expect(result.firstName).toBe(u.firstName); - }, - (error) => { - throw error; - }, - ); - - let req = httpMock.expectOne((request: HttpRequest): boolean => { - expect(request.url).toEqual('http://localhost:3000/api/users/1'); - expect(request.method).toBe('PUT'); - return true; - }); - req.flush(u); - - u.firstName = 'andrew'; + // The API answers a PUT with the saved user as snake_case JSON. Flush a + // literal in that shape whose values differ from the local `u`, so the + // assertions only pass if the snake_case -> camelCase mapping ran. Flushing + // `u` back, as this test used to, could not tell a working mapping from a + // broken one because both sides already held the same camelCase values. userService.update(u).subscribe({ next: (result) => { expect(result.firstName).toBe('andrew'); + expect(result.lastName).toBe('cain'); + expect(result.nickname).toBe('andy'); + expect(result.receiveTaskNotifications).toBe(true); }, error: (error) => { throw error; }, }); - req = httpMock.expectOne((request: HttpRequest): boolean => { - expect(request.url).toEqual('http://localhost:3000/api/users/1'); - expect(request.method).toBe('PUT'); - return true; - }); - - req.flush(u); - }); - - it('should cache the result of a get request', () => { - const user = new User(); - user.id = 1; - user.lastName = 'renzella'; - user.firstName = 'Jake'; - user.nickname = 'jake'; - user.hasRunFirstTimeSetup = false; - user.email = 'jake@jake.jake'; - user.studentId = '1'; - user.username = 'test'; - user.optInToResearch = true; - user.receivePortfolioNotifications = false; - user.receiveFeedbackNotifications = false; - user.receiveTaskNotifications = false; - - userService.get(1).subscribe(); - const req = httpMock.expectOne((request: HttpRequest): boolean => { expect(request.url).toEqual('http://localhost:3000/api/users/1'); - expect(request.method).toBe('GET'); + expect(request.method).toBe('PUT'); return true; }); - const user2 = user; - user2.id = 1; - req.flush(user2); - - userService.get(1).subscribe(); - - httpMock.expectNone((_request: HttpRequest): boolean => { - return true; + req.flush({ + id: 1, + first_name: 'andrew', + last_name: 'cain', + nickname: 'andy', + email: 'jake@jake.jake', + student_id: '1', + username: 'test', + opt_in_to_research: true, + has_run_first_time_setup: false, + receive_portfolio_notifications: false, + receive_feedback_notifications: false, + receive_task_notifications: true, }); }); - it('should cache fetch/get', () => { - let user = new User(); - user.id = 1; - user.lastName = 'renzella'; - user.firstName = 'Jake'; - user.nickname = 'jake'; - user.hasRunFirstTimeSetup = false; - user.email = 'jake@jake.jake'; - user.studentId = '1'; - user.username = 'test'; - user.optInToResearch = true; - user.receivePortfolioNotifications = false; - user.receiveFeedbackNotifications = false; - user.receiveTaskNotifications = false; + it('maps a snake_case response once and reuses the cached user on get', () => { + let fetchedUser: User; + userService.get(1).subscribe((user) => (fetchedUser = user)); - // 1 request here - userService.get(1).subscribe((data) => { - user = data; + const request = httpMock.expectOne('http://localhost:3000/api/users/1'); + expect(request.request.method).toBe('GET'); + request.flush({ + id: 1, + first_name: 'andrew', + last_name: 'cain', + student_id: 'student-1', + has_run_first_time_setup: true, + receive_feedback_notifications: true, }); - let req = httpMock.expectOne((request: HttpRequest): boolean => { - expect(request.url).toEqual('http://localhost:3000/api/users/1'); - expect(request.method).toBe('GET'); - return true; + expect(fetchedUser).toMatchObject({ + id: 1, + firstName: 'andrew', + lastName: 'cain', + studentId: 'student-1', + hasRunFirstTimeSetup: true, + receiveFeedbackNotifications: true, }); - const user2 = new User(); - Object.keys(user).forEach((key) => (user2[key] = user[key])); - user2.id = 1; - req.flush(user2); - - let user3: User; + let cachedUser: User; + userService.get(1).subscribe((user) => (cachedUser = user)); + expect(cachedUser).toBe(fetchedUser); + expect(cachedUser.firstName).toBe('andrew'); + httpMock.expectNone('http://localhost:3000/api/users/1'); + }); - // 1 request here - userService.fetch(1).subscribe((data) => { - expect(data).toBe(user); - user3 = data; + it('maps a refreshed response onto the same cached user', () => { + let cachedUser: User; + userService.get(1).subscribe((user) => (cachedUser = user)); + const initialRequest = httpMock.expectOne('http://localhost:3000/api/users/1'); + expect(initialRequest.request.method).toBe('GET'); + initialRequest.flush({ + id: 1, + first_name: 'andrew', + last_name: 'cain', + student_id: 'student-1', + receive_feedback_notifications: true, }); + expect(cachedUser.firstName).toBe('andrew'); - req = httpMock.expectOne((request: HttpRequest): boolean => { - expect(request.url).toEqual('http://localhost:3000/api/users/1'); - expect(request.method).toBe('GET'); - return true; + let refreshedUser: User; + userService.fetch(1).subscribe((user) => (refreshedUser = user)); + const refreshRequest = httpMock.expectOne('http://localhost:3000/api/users/1'); + expect(refreshRequest.request.method).toBe('GET'); + refreshRequest.flush({ + id: 1, + first_name: 'fred', + last_name: 'smith', + student_id: 'student-2', + receive_feedback_notifications: false, }); - const user4 = new User(); - Object.keys(user2).forEach((key) => (user4[key] = user2[key])); - user4.firstName = 'fred'; - req.flush(user4); - expect(user3).toBe(user); - - httpMock.expectNone((_request: HttpRequest): boolean => { - return true; + expect(refreshedUser).toBe(cachedUser); + expect(cachedUser).toMatchObject({ + firstName: 'fred', + lastName: 'smith', + studentId: 'student-2', + receiveFeedbackNotifications: false, }); + userService.get(1).subscribe((user) => expect(user).toBe(refreshedUser)); + httpMock.expectNone('http://localhost:3000/api/users/1'); }); }); diff --git a/src/app/api/services/task-similarity.service.ts b/src/app/api/services/task-similarity.service.ts index d002a3e68d..1578355df2 100644 --- a/src/app/api/services/task-similarity.service.ts +++ b/src/app/api/services/task-similarity.service.ts @@ -28,7 +28,6 @@ export class TaskSimilarityService extends CachedEntityService { this.mapping.addJsonKey('flagged'); } - // eslint-disable-next-line @typescript-eslint/no-unused-vars public override createInstanceFrom(_json: object, constructorParams: Task): TaskSimilarity { return new TaskSimilarity(constructorParams); } diff --git a/src/app/api/services/task.service.ts b/src/app/api/services/task.service.ts index e82b781f12..659a60f28d 100644 --- a/src/app/api/services/task.service.ts +++ b/src/app/api/services/task.service.ts @@ -17,6 +17,7 @@ import {MappingFunctions} from './mapping-fn'; @Injectable() export class TaskService extends CachedEntityService { public readonly taskStatusUpdated$: EventEmitter = new EventEmitter(); + public readonly taskSubmissionCompleted$: EventEmitter = new EventEmitter(); protected readonly endpointFormat = '/projects/:projectId:/task_def_id/:taskDefId:'; @@ -58,6 +59,10 @@ export class TaskService extends CachedEntityService { keys: 'targetDueDate', toEntityFn: MappingFunctions.mapDateToEndOfDay, }, + {keys: 'effectiveDeadline', toEntityFn: MappingFunctions.mapDate}, + {keys: 'effectiveDeadlineDate', toEntityFn: MappingFunctions.mapDateToDay}, + 'effectiveDeadlineReason', + 'effectiveDeadlineSourceId', 'extensions', 'scormExtensions', { @@ -74,6 +79,7 @@ export class TaskService extends CachedEntityService { 'includeInPortfolio', 'similarityFlag', 'numNewComments', + 'hasFeedback', 'trigger', 'hasExtensions', 'pinned', @@ -243,6 +249,13 @@ export class TaskService extends CachedEntityService { this.taskStatusUpdated$.emit(task); } + public notifyTransitionComplete(task: Task, submissionCompleted: boolean): void { + this.notifyStatusChange(task); + if (submissionCompleted) { + this.taskSubmissionCompleted$.emit(task); + } + } + public readonly statusColors: Map = TaskStatus.STATUS_COLORS; public readonly switchableStates = TaskStatus.SWITCHABLE_STATES; diff --git a/src/app/api/services/user.service.ts b/src/app/api/services/user.service.ts index c63e903b2b..bd1620e9fe 100644 --- a/src/app/api/services/user.service.ts +++ b/src/app/api/services/user.service.ts @@ -4,6 +4,7 @@ import {Injectable} from '@angular/core'; import {Observable} from 'rxjs'; import {UnitRole, UnitService, User} from 'src/app/api/models/doubtfire-model'; import {AppInjector} from 'src/app/app-injector'; +import {type ThemePreference} from 'src/app/common/theme/theme.service'; import API_URL from 'src/app/config/constants/apiUrl'; @Injectable() @@ -27,10 +28,14 @@ export class UserService extends CachedEntityService { 'email', 'username', 'nickname', + 'displayName', 'systemRole', 'receiveTaskNotifications', 'receivePortfolioNotifications', 'receiveFeedbackNotifications', + 'displayPeerProgress', + 'themePreference', + 'themePreferenceUpdatedAt', 'hasRunFirstTimeSetup', 'pronouns', 'acceptedTiiEula', @@ -38,7 +43,10 @@ export class UserService extends CachedEntityService { this._currentUser = this.anonymousUser; - this.mapping.mapAllKeysToJsonExcept('id'); + // Theme sync has a deliberately narrow writer below. Profile updates must + // not replay a stale account timestamp or turn a missing preference into an + // accidental choice. + this.mapping.mapAllKeysToJsonExcept('id', 'themePreference', 'themePreferenceUpdatedAt'); } // eslint-disable-next-line @typescript-eslint/no-unused-vars, @typescript-eslint/no-explicit-any @@ -75,6 +83,13 @@ export class UserService extends CachedEntityService { return this.query(undefined, {endpointFormat: this.tutorEndpointFormat}); } + public updateThemePreference(user: User, preference: ThemePreference): Observable { + return this.update(user, { + entity: user, + body: {user: {theme_preference: preference}}, + }); + } + public adminRoleFor(unitId: number, user: User): UnitRole { return this.adminOrAuditorRoleFor('Admin', unitId, user); } diff --git a/src/app/app.component.html b/src/app/app.component.html index 53e25cc7c8..b42275d198 100644 --- a/src/app/app.component.html +++ b/src/app/app.component.html @@ -1,3 +1,9 @@ - - +
+ + + + +
+ + diff --git a/src/app/app.routes.spec.ts b/src/app/app.routes.spec.ts new file mode 100644 index 0000000000..200df02905 --- /dev/null +++ b/src/app/app.routes.spec.ts @@ -0,0 +1,32 @@ +import {describe, expect, it} from 'vitest'; +import {Route, UrlSegment, UrlSegmentGroup} from '@angular/router'; +import {projectDashboardTaskMatcher} from './app.routes'; + +const match = (paths: string[]) => { + const segments = paths.map((path) => new UrlSegment(path, {})); + return { + result: projectDashboardTaskMatcher(segments, {} as UrlSegmentGroup, {} as Route), + segments, + }; +}; + +describe('project dashboard task route matcher', () => { + it('uses one reusable route for task details and feedback destinations', () => { + const taskRoute = match(['dashboard', '1.1P']); + const feedbackRoute = match(['dashboard', '1.1P', 'feedback']); + + expect(taskRoute.result?.consumed).toEqual(taskRoute.segments); + expect(taskRoute.result?.posParams?.['taskAbbreviation'].path).toBe('1.1P'); + expect(taskRoute.result?.posParams?.['mobilePane']).toBeUndefined(); + + expect(feedbackRoute.result?.consumed).toEqual(feedbackRoute.segments); + expect(feedbackRoute.result?.posParams?.['taskAbbreviation'].path).toBe('1.1P'); + expect(feedbackRoute.result?.posParams?.['mobilePane'].path).toBe('feedback'); + }); + + it('does not consume the dashboard root or unrelated suffixes', () => { + expect(match(['dashboard']).result).toBeNull(); + expect(match(['dashboard', '1.1P', 'details']).result).toBeNull(); + expect(match(['dashboard', '1.1P', 'feedback', 'extra']).result).toBeNull(); + }); +}); diff --git a/src/app/app.routes.ts b/src/app/app.routes.ts index c9160fca0e..4181f99bf6 100644 --- a/src/app/app.routes.ts +++ b/src/app/app.routes.ts @@ -1,12 +1,17 @@ -import {Routes} from '@angular/router'; +import {Routes, UrlMatcher} from '@angular/router'; import {EditProfileComponent} from './account/edit-profile/edit-profile.component'; import {InstitutionSettingsComponent} from './admin/institution-settings/institution-settings.component'; import {FUnitsComponent} from './admin/states/units/units.component'; import {FUsersComponent} from './admin/states/users/users.component'; import {roleWhitelistGuard} from './common/guards/role-whitelist.guard'; +import {NotificationsPageComponent} from './common/notifications-page/notifications-page.component'; import {ScormPlayerComponent} from './common/scorm-player/scorm-player.component'; import {SubmissionFilesDownloadComponent} from './common/submission-files-download/submission-files-download.component'; import {SuccessCloseComponent} from './common/success-close/success-close.component'; +import {courseFlowDirtyGuard} from './courseflow/coursemap/course-flow-dirty.guard'; +import {CrossDashboardComponent} from './dashboard/f-cross-dashboard.component'; +import {DemoControlsComponent} from './demo/demo-controls/demo-controls.component'; +import {demoToolsGuard} from './demo/demo-tools.guard'; import {TimeoutComponent} from './errors/states/timeout/timeout.component'; import {UnauthorisedComponent} from './errors/states/unauthorised/unauthorised.component'; import {AcceptEulaComponent} from './eula/accept-eula/accept-eula.component'; @@ -35,6 +40,24 @@ import {UnitRootStateComponent} from './units/unit-root-state.component'; import {resolveUnit} from './units/unit.resolver'; import {WelcomeComponent} from './welcome/welcome.component'; +export const projectDashboardTaskMatcher: UrlMatcher = (segments) => { + if (segments.length < 2 || segments.length > 3 || segments[0].path !== 'dashboard') { + return null; + } + + if (segments.length === 3 && segments[2].path !== 'feedback') { + return null; + } + + return { + consumed: segments, + posParams: { + taskAbbreviation: segments[1], + ...(segments[2] ? {mobilePane: segments[2]} : {}), + }, + }; +}; + export const routes: Routes = [ {path: '', pathMatch: 'full', redirectTo: 'home'}, {path: 'home', component: HomeComponent}, @@ -44,9 +67,45 @@ export const routes: Routes = [ {path: 'timeout', component: TimeoutComponent}, {path: 'success-close', component: SuccessCloseComponent}, {path: 'edit_profile', component: EditProfileComponent}, + {path: 'notifications', component: NotificationsPageComponent}, + ...['coursemap', 'coursemap/:courseMapId'].map((path) => ({ + path, + loadComponent: () => + import('./courseflow/coursemap/coursemap.component').then((m) => m.CoursemapComponent), + canActivate: [roleWhitelistGuard], + canDeactivate: [courseFlowDirtyGuard], + data: { + pageTitle: 'Course Flow', + roleWhitelist: ['Student', 'Tutor', 'Convenor', 'Admin', 'Auditor'], + }, + })), + { + path: 'unit-hub', + loadComponent: () => import('./unit-hub/unit-hub.component').then((m) => m.UnitHubComponent), + canActivate: [roleWhitelistGuard], + data: { + pageTitle: 'Unit Hub', + roleWhitelist: ['Student', 'Tutor', 'Convenor', 'Admin', 'Auditor'], + }, + }, + { + path: 'theme-demo', + loadComponent: () => + import('./common/theme/theme-demo.component').then((m) => m.ThemeDemoComponent), + data: {pageTitle: 'Theme foundation'}, + }, + { + path: 'demo-controls', + component: DemoControlsComponent, + canActivate: [demoToolsGuard], + data: {pageTitle: 'Demo controls'}, + }, {path: 'eula', component: AcceptEulaComponent}, {path: 'lti', component: LtiDashboardComponent}, {path: 'lti/link', component: LtiUnitLinkComponent}, + // Deliberately open: this is an empty iframe onto the bundled JPlag viewer at /JPlag/. It + // makes no api call and shows nothing until staff hand it a blob url that only exists in + // their own tab, so there is nothing here to guard. Listed in DELIBERATELY_OPEN_PATHS. {path: 'jplag-report-viewer', component: JplagReportViewerComponent}, { path: 'projects/:projectId/task_def_id/:taskDefId/scorm-player/normal', @@ -67,8 +126,20 @@ export const routes: Routes = [ path: 'projects/:projectId/task_def_id/:taskDefId/submission_files/download', component: SubmissionFilesDownloadComponent, }, - {path: 'view-all-units', component: FUnitsComponent, data: {mode: 'tutor'}}, + { + path: 'view-all-units', + component: FUnitsComponent, + canActivate: [roleWhitelistGuard], + data: {mode: 'tutor', roleWhitelist: ['Tutor', 'Convenor', 'Admin', 'Auditor']}, + }, + // Deliberately open: 'student' mode lists the signed in user's own enrolled units. {path: 'view-all-projects', component: FUnitsComponent, data: {mode: 'student'}}, + { + path: 'dashboard', + component: CrossDashboardComponent, + canActivate: [roleWhitelistGuard], + data: {roleWhitelist: ['Student'], pageTitle: 'Dashboard'}, + }, { path: 'admin/units', component: FUnitsComponent, @@ -99,10 +170,18 @@ export const routes: Routes = [ canActivate: [roleWhitelistGuard], data: {task: 'Discussion', roleWhitelist: ['Admin', 'Auditor', 'Tutor']}, }, + // Top level, so the guard checks the SYSTEM role here, not a unit role. Convenor is in the + // list because this url is open to system convenors today and guarding it must not take that + // away. It is a wider list than 'tutor-discussion' above, which this change does not touch. { path: 'tutor-attendance', component: TutorDiscussionComponent, - data: {attendance: true, task: 'Check-in'}, + canActivate: [roleWhitelistGuard], + data: { + attendance: true, + task: 'Check-in', + roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor'], + }, }, { path: 'units', @@ -116,29 +195,66 @@ export const routes: Routes = [ }, children: [ {path: '', pathMatch: 'full', redirectTo: 'tasks/inbox'}, - {path: 'analytics', component: UnitAnalyticsComponent, data: {task: 'Unit Analytics'}}, - {path: 'students/groups', component: UnitGroupsComponent, data: {task: 'Student Groups'}}, + { + path: 'analytics', + component: UnitAnalyticsComponent, + canActivate: [roleWhitelistGuard], + data: { + task: 'Unit Analytics', + roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor'], + }, + }, + { + path: 'students/groups', + component: UnitGroupsComponent, + canActivate: [roleWhitelistGuard], + data: { + task: 'Student Groups', + roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor'], + }, + }, { path: 'students/portfolios', component: PortfoliosComponent, - data: {task: 'Student Portfolios'}, + canActivate: [roleWhitelistGuard], + data: { + task: 'Student Portfolios', + roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor'], + }, }, { path: 'students/portfolios/:projectId', component: PortfoliosComponent, - data: {task: 'Student Portfolios'}, + canActivate: [roleWhitelistGuard], + data: { + task: 'Student Portfolios', + roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor'], + }, }, { path: 'students/portfolios/:projectId/:tab', component: PortfoliosComponent, - data: {task: 'Student Portfolios'}, + canActivate: [roleWhitelistGuard], + data: { + task: 'Student Portfolios', + roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor'], + }, }, { path: 'students/portfolios/:projectId/:tab/:taskAbbreviation', component: PortfoliosComponent, - data: {task: 'Student Portfolios'}, + canActivate: [roleWhitelistGuard], + data: { + task: 'Student Portfolios', + roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor'], + }, + }, + { + path: 'students', + component: StudentsListComponent, + canActivate: [roleWhitelistGuard], + data: {task: 'Student List', roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor']}, }, - {path: 'students', component: StudentsListComponent, data: {task: 'Student List'}}, { path: 'admin', component: UnitAdminStateComponent, @@ -151,12 +267,31 @@ export const routes: Routes = [ canActivate: [roleWhitelistGuard], data: {task: 'Unit Administration', roleWhitelist: ['Convenor', 'Admin', 'Auditor']}, }, - {path: 'rollover', component: RolloverComponent, data: {task: 'Unit Rollover'}}, - {path: 'discussion', component: TutorDiscussionComponent, data: {task: 'Discussion'}}, + { + path: 'rollover', + component: RolloverComponent, + canActivate: [roleWhitelistGuard], + data: {task: 'Unit Rollover', roleWhitelist: ['Convenor', 'Admin', 'Auditor']}, + }, + // Convenor is in both lists because the staff menu offers Discussion and Check-in to + // every unit role, and the header QR button routes non students straight to + // 'discussion'. Do not narrow these to the 'tutor-discussion' list at the top level: + // that one is matched against the system role, this one against the unit role. + { + path: 'discussion', + component: TutorDiscussionComponent, + canActivate: [roleWhitelistGuard], + data: {task: 'Discussion', roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor']}, + }, { path: 'check-in', component: TutorDiscussionComponent, - data: {attendance: true, task: 'Check-in'}, + canActivate: [roleWhitelistGuard], + data: { + attendance: true, + task: 'Check-in', + roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor'], + }, }, { path: 'tasks', @@ -166,6 +301,7 @@ export const routes: Routes = [ canActivate: [roleWhitelistGuard], }, { + // The guard on this parent runs for every inbox child below it. path: 'tasks', canActivate: [roleWhitelistGuard], data: {roleWhitelist: ['Convenor', 'Admin', 'Auditor', 'Tutor']}, @@ -214,10 +350,15 @@ export const routes: Routes = [ }, ], }, + // Deliberately the same list as the 'tasks' viewer above, which is narrower than the + // inbox children. This is the whole unit task list, not one tutor's queue. A deep link + // meant for a tutor belongs on 'tasks/inbox/:studentId/:taskDefAbbr', which does + // whitelist Tutor. { path: 'tasks/:taskAbbreviation', component: TaskViewerStateComponent, - data: {task: 'Task Lists'}, + canActivate: [roleWhitelistGuard], + data: {task: 'Task Lists', roleWhitelist: ['Convenor', 'Admin', 'Auditor']}, }, ], }, @@ -241,7 +382,7 @@ export const routes: Routes = [ data: {task: 'Dashboard'}, }, { - path: 'dashboard/:taskAbbreviation', + matcher: projectDashboardTaskMatcher, component: ProjectDashboardComponent, data: {task: 'Dashboard'}, }, diff --git a/src/app/common/audio-player/audio-player.component.html b/src/app/common/audio-player/audio-player.component.html index 4b4bf2f632..cca85a20a3 100644 --- a/src/app/common/audio-player/audio-player.component.html +++ b/src/app/common/audio-player/audio-player.component.html @@ -1,4 +1,4 @@ -
+
- +
+ +
+ } +
diff --git a/src/app/common/empty-state/empty-state.component.scss b/src/app/common/empty-state/empty-state.component.scss new file mode 100644 index 0000000000..5d4e87f30f --- /dev/null +++ b/src/app/common/empty-state/empty-state.component.scss @@ -0,0 +1,3 @@ +:host { + display: block; +} diff --git a/src/app/common/empty-state/empty-state.component.spec.ts b/src/app/common/empty-state/empty-state.component.spec.ts new file mode 100644 index 0000000000..d7fc454406 --- /dev/null +++ b/src/app/common/empty-state/empty-state.component.spec.ts @@ -0,0 +1,72 @@ +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; +import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {NoopAnimationsModule} from '@angular/platform-browser/animations'; +import {EmptyStateComponent} from './empty-state.component'; + +describe('EmptyStateComponent', () => { + let component: EmptyStateComponent; + let fixture: ComponentFixture; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [EmptyStateComponent, NoopAnimationsModule], + }).compileComponents(); + + fixture = TestBed.createComponent(EmptyStateComponent); + component = fixture.componentInstance; + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('renders the icon, heading and message it is given', () => { + component.icon = 'inbox'; + component.heading = 'Nothing here yet'; + component.message = 'New items will show up in this list.'; + fixture.detectChanges(); + + const root: HTMLElement = fixture.nativeElement; + expect(root.querySelector('mat-icon').textContent.trim()).toBe('inbox'); + expect(root.textContent).toContain('Nothing here yet'); + expect(root.textContent).toContain('New items will show up in this list.'); + }); + + it('renders no action when no action label is given', () => { + component.icon = 'inbox'; + component.heading = 'Nothing here yet'; + component.message = 'New items will show up in this list.'; + fixture.detectChanges(); + + expect(fixture.nativeElement.querySelector('button')).toBeNull(); + }); + + it('renders the action and emits it on click, only when a label is given', () => { + component.icon = 'inbox'; + component.heading = 'Nothing here yet'; + component.message = 'New items will show up in this list.'; + component.actionLabel = 'Refresh'; + fixture.detectChanges(); + + const button: HTMLButtonElement = fixture.nativeElement.querySelector('button'); + expect(button).not.toBeNull(); + expect(button.textContent).toContain('Refresh'); + + const emitted = vi.fn(); + component.action.subscribe(emitted); + button.click(); + + expect(emitted).toHaveBeenCalledTimes(1); + }); + + it('marks the icon decorative so it is not announced twice alongside the heading', () => { + component.icon = 'inbox'; + component.heading = 'Nothing here yet'; + component.message = 'New items will show up in this list.'; + fixture.detectChanges(); + + expect(fixture.nativeElement.querySelector('mat-icon').getAttribute('aria-hidden')).toBe( + 'true', + ); + }); +}); diff --git a/src/app/common/empty-state/empty-state.component.ts b/src/app/common/empty-state/empty-state.component.ts new file mode 100644 index 0000000000..e9a2004ecc --- /dev/null +++ b/src/app/common/empty-state/empty-state.component.ts @@ -0,0 +1,28 @@ +import {ChangeDetectionStrategy, Component, EventEmitter, Input, Output} from '@angular/core'; +import {MatButtonModule} from '@angular/material/button'; +import {MatIconModule} from '@angular/material/icon'; + +/** + * A centered icon, heading, message and optional action for a collection that + * has nothing in it. Render it only when the collection is confirmed empty, + * not while it is still loading and not when the load failed - "you have + * none" and "we could not find out" are different states and only one of them + * has a way out. + */ +@Component({ + selector: 'f-empty-state', + standalone: true, + imports: [MatIconModule, MatButtonModule], + templateUrl: './empty-state.component.html', + styleUrl: './empty-state.component.scss', + changeDetection: ChangeDetectionStrategy.Eager, +}) +export class EmptyStateComponent { + @Input() icon: string; + @Input() heading: string; + @Input() message: string; + + /** Omit to render no action. */ + @Input() actionLabel?: string; + @Output() action: EventEmitter = new EventEmitter(); +} diff --git a/src/app/common/entity-form/entity-form.component.spec.ts b/src/app/common/entity-form/entity-form.component.spec.ts new file mode 100644 index 0000000000..8faff9504f --- /dev/null +++ b/src/app/common/entity-form/entity-form.component.spec.ts @@ -0,0 +1,44 @@ +import {describe, expect, it, vi} from 'vitest'; +import {EntityFormComponent} from './entity-form.component'; + +// EntityFormComponent is an abstract base. We exercise submit() on a bare +// instance and stub the collaborators the two feedback branches touch, rather +// than standing a concrete subclass up in a TestBed. +interface BareEntityForm { + formData: unknown; + selected: unknown; + hasChanges: () => boolean; + entityName: string; + submit(service: unknown, alertService: unknown, success: unknown): void; +} + +describe('EntityFormComponent submit feedback', () => { + const bare = () => Object.create(EntityFormComponent.prototype) as BareEntityForm; + + it('uses a neutral message, not a red error, when nothing changed', () => { + const component = bare(); + component.formData = {valid: true}; + component.selected = {id: 1}; + component.hasChanges = () => false; + component.entityName = 'Tutorial'; + const alertService = {message: vi.fn(), error: vi.fn(), success: vi.fn()}; + + component.submit({}, alertService, vi.fn()); + + expect(alertService.message).toHaveBeenCalledWith('Tutorial was not changed', 6000); + expect(alertService.error).not.toHaveBeenCalled(); + }); + + it('tells the user the save was blocked when the form is invalid', () => { + const component = bare(); + const markAllAsTouched = vi.fn(); + component.formData = {valid: false, markAllAsTouched}; + component.entityName = 'Tutorial'; + const alertService = {message: vi.fn(), error: vi.fn(), success: vi.fn()}; + + component.submit({}, alertService, vi.fn()); + + expect(markAllAsTouched).toHaveBeenCalled(); + expect(alertService.error).toHaveBeenCalledWith('Check the highlighted fields', 6000); + }); +}); diff --git a/src/app/common/entity-form/entity-form.component.ts b/src/app/common/entity-form/entity-form.component.ts index 8b9eddb11d..c99ffc2787 100644 --- a/src/app/common/entity-form/entity-form.component.ts +++ b/src/app/common/entity-form/entity-form.component.ts @@ -144,8 +144,9 @@ export abstract class EntityFormComponent implements AfterView const data = this.formDataToNewObject(this.serverKey); // sent as path id and body response = service.create(data, this.optionsOnRequest('create')); } else { - // Nothing has changed if the selected value, so we want to inform the user - alertService.error(`${this.entityName} was not changed`, 6000); + // Nothing has changed on the selected value. This is not a failure, so + // use a neutral message rather than a red error snackbar. + alertService.message(`${this.entityName} was not changed`, 6000); return; } @@ -175,8 +176,11 @@ export abstract class EntityFormComponent implements AfterView }); } else { // Once we mark forms as touched, erroneous state will be rendered - // In the form's template accordingly + // In the form's template accordingly. Tell the user why the save did + // nothing, otherwise the button reads as dead when the invalid control + // is scrolled off screen. this.formData.markAllAsTouched(); + alertService.error('Check the highlighted fields', 6000); } } diff --git a/src/app/common/filters/filters.coffee b/src/app/common/filters/filters.coffee deleted file mode 100644 index 3a9711cd0b..0000000000 --- a/src/app/common/filters/filters.coffee +++ /dev/null @@ -1,320 +0,0 @@ -# Component is no longer used and is unlinked from the app -# This file is left for reference, as these are migrated into Pipes - -angular.module("doubtfire.common.filters", []) - -# -# Paging filter - start from the indicated index -# -.filter('startFrom', -> - (input, start) -> - input?.slice(+start) -) - -.filter('showStudents', -> - (input, kind, tutor) -> - if input - if kind == "mine" || kind == "myStudents" - _.filter input, (project) -> project.hasTutor(tutor) - else - input - else - input -) - -.filter('orderObjectBy', -> - (items, field, reverse) -> - filtered = [] - - angular.forEach items, (item) -> - filtered.push(item) - - filtered.sort (a, b) -> - a[field] > b[field] ? 1 : -1 - - if reverse - filtered.reverse() - - filtered -) - -.filter('byGrade', -> - (input, grade) -> - if input - _.filter input, (task) -> (task?) && task.definition.targetGrade <= grade - else - input -) - -.filter('studentsWithPlagiarism', -> - (input) -> - if input - _.filter input, (student) -> (student?) && student.similarityFlag - else - input -) - -.filter('studentsWithPortfolio', -> - (input, option) -> - if input - _.filter input, (student) -> option == 'allStudents' || ((student?) && student.hasPortfolio > 0) - else - input -) - -.filter('studentsWithSubmittedGrade', -> - (input, grade) -> - if input && grade > -1 - _.filter input, (student) -> (student?) && student.submittedGrade == grade - else - input -) - -.filter('taskWithPlagiarism', -> - (input) -> - if input - _.filter input, (task) -> (task?) && task.similarityFlag - else - input -) - -.filter('studentsForGroup', -> - (input, gs, group, members) -> - if input - if gs.keepGroupsInSameClass - _.filter input, (student) -> (student?) && (student.isEnrolledIn(group.tutorial)) && (not _.find(members, (mbr) -> student.id == mbr.id )) - else - _.filter input, (student) -> (student?) && not _.find(members, (mbr) -> student.id == mbr.id ) - else - input -) - -.filter('outcomeFilter', -> - (input, outcomeId) -> - if input && outcomeId - _.filter input, (item) -> item.learningOutcome?.id == outcomeId - else - input -) - -.filter('taskDefinitionFilter', -> - (input, taskDefId) -> - if input && taskDefId - _.filter input, (item) -> item.definition.id == taskDefId - else - input -) - -.filter('truncatedMarkdown', ($filter) -> - (input, truncateTo) -> - truncateTo = truncateTo or 128 - input = $filter('markdown')(input) - input = $filter('stripTags')(input) - $filter('truncate')(input, truncateTo, '...') -) - -.filter('statusFilter', -> - (input, statusKind) -> - if input && statusKind - _.filter input, (task) -> - if statusKind == 'discuss' - task.status == statusKind || task.status == 'demonstrate' - else - task.status == statusKind - else - input -) - -.filter('projectFilter', -> - (input, text) -> - if _.isString(text) && text.length > 0 && input - matchText = text.toLowerCase() - _.filter input, (project) -> (project?) && project.matches(matchText) - else - input -) - -.filter('projectUnitFilter', -> - (input, text) -> - if _.isString(text) && text.length > 0 && input - matchText = text.toLowerCase() - _.filter input, (project) -> (project?) && project.unit.matches(matchText) - else - input -) - -.filter('unitFilter', -> - (input, text) -> - if _.isString(text) && text.length > 0 && input - matchText = text.toLowerCase() - _.filter input, (unit) -> (unit?) && unit.matches(matchText) - else - input -) - -.filter('teachingPeriodFilter', -> - (input, text) -> - if _.isString(text) && text.length > 0 && input - matchText = text.toLowerCase() - _.filter input, (tp) -> (tp?) && tp.period.toLowerCase().indexOf(matchText) >= 0 - else - input -) - - -.filter('taskForPortfolio', (newTaskService) -> - (input, apply) -> - if (! apply) || (! input) - input - else - _.filter input, (task) -> - if task? - !_.includes(newTaskService.toBeWorkedOn, task.status) - else - false -) - -# NEW FILTERS - -.filter('tasksOfTaskDefinition', -> - (tasks, taskDefinition) -> - return tasks unless (taskDefinition? && tasks?) - tasks = _.filter tasks, (task) -> task.definition.id == taskDefinition.id -) - -.filter('tasksWithStatuses', -> - (tasks, statusKeys) -> - return tasks unless tasks? - return [] if _.isEmpty statusKeys - _.filter tasks, (task) -> _.includes(statusKeys, task.status) -) - -.filter('tasksInTutorials', -> - (tasks, tutorialIds) -> - return tasks unless tasks? - return [] if _.isEmpty tutorialIds - # If task is group task, then use group tutorial, else use project tutorial - _.filter tasks, (task) -> - if task.isGroupTask() - if task.group()? - _.includes(tutorialIds, task.group.tutorial.id) - else - _.filter(task.project.tutorials, (tutorial) -> - _.includes(tutorialIds, tutorial.id) - ).length > 0 - -) - -.filter('tasksWithSearchText', -> - (tasks, searchText) -> - return tasks unless (searchText? && tasks?) - searchText = searchText.toLowerCase() - _.filter tasks, (task) -> - p = task.project - p.matches(searchText) -) - -.filter('tutorialCampusFilter', -> - (tutorials, project) -> - return tutorials unless project? - _.filter tutorials, (tute) -> - !project.campus?.id? || !tute.campus? || tute.campus.id == project.campus.id -) - -.filter('groupsInTutorials', -> - (input, unitRole, kind) -> - return unless input? && unitRole? && kind? - if kind == 'mine' - return _.filter(input, (group) -> group.tutorial.tutor.id == unitRole.user.id) - return input -) - -.filter('groupsForStudent', -> - (input, project, groupSet) -> - # Filter by tutorial if keep groups in same class - return input unless input? && groupSet? && project? - grp = project.groupForGroupSet(groupSet) - return [grp] if grp - return input unless groupSet.keepGroupsInSameClass - _.filter(input, (group) -> project.isEnrolledIn(group.tutorial)) -) - -.filter('groupsWithName', -> - (input, searchName) -> - return input unless (searchName? && input?) - searchName = searchName.toLowerCase() - _.filter(input, (group) -> - # Search using name or abbreviation - group.name.toLowerCase().indexOf(searchName) >= 0 - ) -) - -.filter('tasksForGroupset', -> - (input, groupSet) -> - return input unless input? - return input.filter (task) -> (task.definition.groupSet == groupSet) || (!task.definition.groupSet? && !groupSet?) -) - -.filter('paginateAndSort', ($filter) -> - (input, pagination, tableSort) -> - return unless input? && tableSort? && pagination? - return input if input.length == 0 - pagination.show = input.length > pagination.pageSize - pagination.totalSize = input.length - input = $filter('orderBy')(input, tableSort.order, tableSort.reverse) - input = $filter('startFrom')(input, (pagination.currentPage - 1) * pagination.pageSize) - input = $filter('limitTo')(input, pagination.pageSize) - input -) - -.filter('tasksWithName', -> - (tasks, searchName) -> - return tasks unless (searchName? && tasks?) - searchName = searchName.toLowerCase() - _.filter(tasks, (task) -> - # Search using name or abbreviation - task.definition.name.toLowerCase().indexOf(searchName) >= 0 || - task.definition.abbreviation.toLowerCase().indexOf(searchName) >= 0 - ) -) - -.filter('taskDefinitionName', -> - (taskDefinitions, searchName) -> - return taskDefinitions unless (searchName? && taskDefinitions?) - searchName = searchName.toLowerCase() - _.filter(taskDefinitions, (td) -> - # Search using name or abbreviation - td.name.toLowerCase().indexOf(searchName) >= 0 || - td.abbreviation.toLowerCase().indexOf(searchName) >= 0 || - td.targetGradeText.toLowerCase().indexOf(searchName) >= 0 - ) -) - -.filter('humanizedDate', ($filter) -> - (input) -> - return unless input? - moment(input).calendar(null, { - sameDay: '', - nextDay: '[tomorrow]', - nextWeek: '[this] dddd', - lastDay: '[yesterday]', - lastWeek: '[last] dddd', - sameElse: 'DD/MM/YYYY' - }) -) - -.filter('lcfirst', -> - (input) -> - return if !input? || input.length == 0 - input[0].toLowerCase() + input.substring(1) -) - -.filter('isActiveUnitRole', -> - (unitRoles) -> - _.filter(unitRoles, (ur) -> ur.active ) -) - -.filter('to_trusted', ($sce) -> - (text) -> - $sce.getTrustedHtml(text) -) diff --git a/src/app/common/guards/role-whitelist.guard.ts b/src/app/common/guards/role-whitelist.guard.ts index 124c7ad937..6e11b8f7cf 100644 --- a/src/app/common/guards/role-whitelist.guard.ts +++ b/src/app/common/guards/role-whitelist.guard.ts @@ -1,4 +1,4 @@ -import {inject} from '@angular/core'; +import {inject, isDevMode} from '@angular/core'; import {ActivatedRouteSnapshot, CanActivateFn, Router, UrlTree} from '@angular/router'; import {Observable, filter, map, of, take} from 'rxjs'; import {AuthenticationService, UserService} from 'src/app/api/models/doubtfire-model'; @@ -14,6 +14,14 @@ export const roleWhitelistGuard: CanActivateFn = ( const roleWhitelist = route.data['roleWhitelist'] as string[] | undefined; if (!roleWhitelist?.length) { + // A route that asks for the guard and gives it no whitelist lets everybody through. Keep + // letting them through so production behaviour does not change, but say so while developing. + if (isDevMode()) { + console.error( + `roleWhitelistGuard is on route '${routePath(route)}' with no roleWhitelist in its data, ` + + 'so it allows every signed in user. Add a roleWhitelist or drop the guard.', + ); + } return of(true); } @@ -29,6 +37,15 @@ export const roleWhitelistGuard: CanActivateFn = ( ); }; +function routePath(route: ActivatedRouteSnapshot): string { + return ( + route.pathFromRoot + .map((snapshot) => snapshot.routeConfig?.path) + .filter((path) => !!path) + .join('/') || '/' + ); +} + function roleForRoute( route: ActivatedRouteSnapshot, userService: UserService, diff --git a/src/app/common/guards/spec/role-whitelist.guard.spec.ts b/src/app/common/guards/spec/role-whitelist.guard.spec.ts new file mode 100644 index 0000000000..63869a7559 --- /dev/null +++ b/src/app/common/guards/spec/role-whitelist.guard.spec.ts @@ -0,0 +1,456 @@ +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; +import {HttpClient} from '@angular/common/http'; +import {Type} from '@angular/core'; +import {TestBed} from '@angular/core/testing'; +import { + ActivatedRouteSnapshot, + Data, + Route, + Router, + RouterStateSnapshot, + Routes, + UrlTree, + convertToParamMap, +} from '@angular/router'; +import {BehaviorSubject, Observable} from 'rxjs'; +import {InstitutionSettingsComponent} from 'src/app/admin/institution-settings/institution-settings.component'; +import {FUnitsComponent} from 'src/app/admin/states/units/units.component'; +import {FUsersComponent} from 'src/app/admin/states/users/users.component'; +import {AuthenticationService, UserService} from 'src/app/api/models/doubtfire-model'; +import {routes} from 'src/app/app.routes'; +import {AlertService} from 'src/app/common/services/alert.service'; +import {DoubtfireConstants} from 'src/app/config/constants/doubtfire-constants'; +import {GlobalStateService} from 'src/app/projects/states/index/global-state.service'; +import {JplagReportViewerComponent} from 'src/app/projects/states/jplag/jplag-report-viewer.component'; +import {TutorDiscussionComponent} from 'src/app/projects/states/tutor-discussion/tutor-discussion.component'; +import {UnitAnalyticsComponent} from 'src/app/units/states/analytics/unit-analytics-route.component'; +import {UnitAdminStateComponent} from 'src/app/units/states/edit/unit-admin-state.component'; +import {UnitGroupsComponent} from 'src/app/units/states/groups/unit-groups/unit-groups.component'; +import {PortfoliosComponent} from 'src/app/units/states/portfolios/portfolios.component'; +import {RolloverComponent} from 'src/app/units/states/rollover/rollover.component'; +import {StudentsListComponent} from 'src/app/units/states/students-list/students-list.component'; +import {UnitTaskInboxStateComponent} from 'src/app/units/states/tasks/inbox/unit-task-inbox-state.component'; +import {TaskViewerStateComponent} from 'src/app/units/task-viewer/task-viewer-state.component'; +import {roleWhitelistGuard} from '../role-whitelist.guard'; + +/** + * Components that only teaching staff should ever reach. Every route that renders one of + * these has to carry roleWhitelistGuard, on itself or on a parent, unless its full path is + * listed in DELIBERATELY_OPEN_PATHS below. + * + * Add to this list when you add a staff screen. That is the whole point of it. + */ +const STAFF_ONLY_COMPONENTS: Type[] = [ + FUnitsComponent, + FUsersComponent, + InstitutionSettingsComponent, + JplagReportViewerComponent, + PortfoliosComponent, + RolloverComponent, + StudentsListComponent, + TaskViewerStateComponent, + TutorDiscussionComponent, + UnitAdminStateComponent, + UnitAnalyticsComponent, + UnitGroupsComponent, + UnitTaskInboxStateComponent, +]; + +/** + * Routes onto a staff component that are open on purpose. Anything added here needs a + * comment on the route itself saying why it is open. + */ +const DELIBERATELY_OPEN_PATHS: string[] = [ + // FUnitsComponent in 'student' mode lists the signed in user's own enrolled units. + 'view-all-projects', + // An empty iframe onto the bundled JPlag viewer. It makes no api call and holds no data of + // its own until staff hand it a blob url from their own tab. + 'jplag-report-viewer', +]; + +interface FlatRoute { + path: string; + component?: Type; + guarded: boolean; + roleWhitelist?: string[]; + ownGuard: boolean; + ownWhitelist?: string[]; +} + +function hasRoleGuard(route: Route): boolean { + return (route.canActivate ?? []).includes(roleWhitelistGuard); +} + +function flatten( + children: Routes, + parentPath: string, + inheritedGuard: boolean, + inheritedWhitelist: string[] | undefined, +): FlatRoute[] { + return children.flatMap((route) => { + const path = [parentPath, route.path].filter((part) => !!part).join('/'); + const ownGuard = hasRoleGuard(route); + const ownWhitelist = route.data?.['roleWhitelist'] as string[] | undefined; + const guarded = inheritedGuard || ownGuard; + const roleWhitelist = ownGuard ? ownWhitelist : inheritedWhitelist; + + const self: FlatRoute = { + path, + component: route.component, + guarded, + roleWhitelist, + ownGuard, + ownWhitelist, + }; + const descendants = route.children ? flatten(route.children, path, guarded, roleWhitelist) : []; + + return [self, ...descendants]; + }); +} + +const flatRoutes = flatten(routes, '', false, undefined); +const staffRoutes = flatRoutes.filter( + (route) => route.component && STAFF_ONLY_COMPONENTS.includes(route.component), +); + +describe('app route role guard coverage', () => { + it('finds the staff routes it is meant to be checking', () => { + // Guards the walk itself. If flatten() ever stops descending, this fails rather than + // letting the assertions below pass over an empty list. + expect(staffRoutes.length).toBeGreaterThanOrEqual(25); + expect(staffRoutes.map((route) => route.path)).toContain( + 'units/:unitId/tasks/:taskAbbreviation', + ); + }); + + it('puts roleWhitelistGuard on every route that renders a staff only component', () => { + const unguarded = staffRoutes + .filter((route) => !route.guarded) + .map((route) => route.path) + .filter((path) => !DELIBERATELY_OPEN_PATHS.includes(path)); + + expect( + unguarded, + `these staff routes have no roleWhitelistGuard: ${unguarded.join(', ')}. Add ` + + 'canActivate: [roleWhitelistGuard] and a roleWhitelist, or list the path in ' + + 'DELIBERATELY_OPEN_PATHS with a comment on the route saying why it is open.', + ).toEqual([]); + }); + + it('gives every guarded staff route a non empty whitelist', () => { + const missing = staffRoutes + .filter((route) => route.guarded && !route.roleWhitelist?.length) + .map((route) => route.path); + + expect(missing, `these guarded routes have no roleWhitelist: ${missing.join(', ')}`).toEqual( + [], + ); + }); + + it('never puts the guard on a route without a whitelist of its own', () => { + // roleWhitelistGuard falls through to true when data.roleWhitelist is missing, so this + // pairing looks protected and is not. + const emptyGuards = flatRoutes + .filter((route) => route.ownGuard && !route.ownWhitelist?.length) + .map((route) => route.path); + + expect( + emptyGuards, + `these routes ask for the guard and give it nothing to check: ${emptyGuards.join(', ')}`, + ).toEqual([]); + }); + + it('leaves the student routes alone', () => { + // The negative case. If a change starts guarding everything, this goes red. + const studentPaths = [ + 'home', + 'sign_in', + 'projects/:projectId/dashboard', + 'projects/:projectId/portfolio', + 'view-all-projects', + ]; + + const wronglyGuarded = flatRoutes + .filter((route) => studentPaths.includes(route.path) && route.guarded) + .map((route) => route.path); + + expect(wronglyGuarded).toEqual([]); + }); + + it('only ever whitelists role names the api actually issues', () => { + const validRoles = ['Student', 'Tutor', 'Convenor', 'Admin', 'Auditor']; + const invalid = flatRoutes + .flatMap((route) => route.ownWhitelist ?? []) + .filter((role) => !validRoles.includes(role)); + + expect(invalid).toEqual([]); + }); +}); + +const UNIT_ID = 12; + +/** + * Which roles the guard has to admit and refuse, per route, run through the guard itself. + * + * The block above only proves a guard is attached. It says nothing about who that guard lets + * in, so a whitelist that locks out a whole role passes it. These do execute the guard, against + * the whitelist that is really in app.routes.ts, so a wrong whitelist goes red here. + * + * Paths under 'units/:unitId' resolve the caller's UNIT role. Top level paths resolve the + * SYSTEM role. Same guard, different question, which is why two identical looking whitelists + * on the two kinds of path do not admit the same people. + */ +const UNIT_ROLE_EXPECTATIONS: {path: string; allow: string[]; refuse: string[]}[] = [ + // The staff menu offers Discussion and Check-in to every unit role, and the header QR button + // routes every non student to 'discussion'. Convenors have to be able to get there. + {path: 'units/:unitId/discussion', allow: ['Convenor', 'Tutor'], refuse: ['Student']}, + {path: 'units/:unitId/check-in', allow: ['Convenor', 'Tutor'], refuse: ['Student']}, + {path: 'units/:unitId/analytics', allow: ['Convenor', 'Tutor'], refuse: ['Student']}, + {path: 'units/:unitId/students', allow: ['Convenor', 'Tutor'], refuse: ['Student']}, + {path: 'units/:unitId/students/groups', allow: ['Convenor', 'Tutor'], refuse: ['Student']}, + {path: 'units/:unitId/students/portfolios', allow: ['Convenor', 'Tutor'], refuse: ['Student']}, + {path: 'units/:unitId/tasks/inbox', allow: ['Convenor', 'Tutor'], refuse: ['Student']}, + {path: 'units/:unitId/tasks/moderation', allow: ['Convenor', 'Tutor'], refuse: ['Student']}, + // Convenor and above. Unit administration, rollover and the whole unit task list are not a + // tutor's queue. A tutor deep link belongs on 'tasks/inbox/:studentId/:taskDefAbbr' above. + {path: 'units/:unitId/admin', allow: ['Convenor'], refuse: ['Tutor', 'Student']}, + {path: 'units/:unitId/rollover', allow: ['Convenor'], refuse: ['Tutor', 'Student']}, + {path: 'units/:unitId/tasks', allow: ['Convenor'], refuse: ['Tutor', 'Student']}, + { + path: 'units/:unitId/tasks/:taskAbbreviation', + allow: ['Convenor'], + refuse: ['Tutor', 'Student'], + }, +]; + +const SYSTEM_ROLE_EXPECTATIONS: {path: string; allow: string[]; refuse: string[]}[] = [ + {path: 'view-all-units', allow: ['Tutor', 'Convenor', 'Admin', 'Auditor'], refuse: ['Student']}, + { + path: 'tutor-attendance', + allow: ['Tutor', 'Convenor', 'Admin', 'Auditor'], + refuse: ['Student'], + }, + // 'tutor-discussion' refuses Convenor on 11.0.x already. This change does not widen it, and + // pinning it here is what makes that a decision rather than an oversight. + {path: 'tutor-discussion', allow: ['Tutor', 'Admin', 'Auditor'], refuse: ['Convenor', 'Student']}, + {path: 'admin/units', allow: ['Convenor', 'Admin', 'Auditor'], refuse: ['Tutor', 'Student']}, + {path: 'admin/users', allow: ['Admin', 'Auditor'], refuse: ['Convenor', 'Tutor', 'Student']}, + { + path: 'admin/institution-settings', + allow: ['Admin', 'Auditor'], + refuse: ['Convenor', 'Tutor', 'Student'], + }, +]; + +/** + * The whitelist the guard will really see on this path, taken from app.routes.ts rather than + * repeated here. Where two routes share a path the first one wins, which is what the router + * does too. + */ +function whitelistFor(path: string): string[] { + const match = flatRoutes.find((route) => route.path === path); + expect(match, `no route in app.routes.ts has the path '${path}'`).toBeDefined(); + expect( + match?.roleWhitelist, + `route '${path}' reaches the guard with no roleWhitelist`, + ).toBeDefined(); + + return match.roleWhitelist; +} + +describe('roleWhitelistGuard role decisions', () => { + const globalState = { + isLoadingSubject: new BehaviorSubject(false), + loadedUnitRoles: {currentValues: [] as {unit: {id: number}; role: string}[]}, + }; + const userService = {currentUser: {role: 'Student'}}; + const router = { + createUrlTree: (commands: unknown[]) => ({commands}) as unknown as UrlTree, + }; + + beforeEach(() => { + globalState.isLoadingSubject.next(false); + globalState.loadedUnitRoles.currentValues = []; + userService.currentUser.role = 'Student'; + + TestBed.configureTestingModule({ + providers: [ + AuthenticationService, + {provide: AlertService, useValue: {}}, + {provide: DoubtfireConstants, useValue: {API_URL: ''}}, + {provide: GlobalStateService, useValue: globalState}, + {provide: HttpClient, useValue: {}}, + {provide: Router, useValue: router}, + {provide: UserService, useValue: userService}, + ], + }); + }); + + afterEach(() => { + TestBed.resetTestingModule(); + }); + + function snapshotFor(data: Data, unitId?: number): ActivatedRouteSnapshot { + return { + data, + paramMap: convertToParamMap(unitId === undefined ? {} : {unitId: String(unitId)}), + parent: null, + pathFromRoot: [], + } as unknown as ActivatedRouteSnapshot; + } + + function activate(snapshot: ActivatedRouteSnapshot): boolean | UrlTree { + const result = TestBed.runInInjectionContext( + () => + roleWhitelistGuard(snapshot, {} as RouterStateSnapshot) as Observable, + ); + + let outcome: boolean | UrlTree | undefined; + result.subscribe((value) => (outcome = value)); + expect(outcome, 'the guard did not settle synchronously').toBeDefined(); + + return outcome; + } + + function admitsUnitRole(path: string, unitRole: string): boolean { + // The system role is left as Student on purpose. If role resolution ever stopped reading the + // unit role it would fall back to this and every allow case below would fail. + userService.currentUser.role = 'Student'; + globalState.loadedUnitRoles.currentValues = + unitRole === 'Student' ? [] : [{unit: {id: UNIT_ID}, role: unitRole}]; + + return activate(snapshotFor({roleWhitelist: whitelistFor(path)}, UNIT_ID)) === true; + } + + function admitsSystemRole(path: string, systemRole: string): boolean { + globalState.loadedUnitRoles.currentValues = []; + userService.currentUser.role = systemRole; + + return activate(snapshotFor({roleWhitelist: whitelistFor(path)})) === true; + } + + for (const expectation of UNIT_ROLE_EXPECTATIONS) { + it(`admits ${expectation.allow.join(' and ')} to ${expectation.path}`, () => { + for (const role of expectation.allow) { + expect(admitsUnitRole(expectation.path, role), `${role} was refused`).toBe(true); + } + }); + + it(`refuses ${expectation.refuse.join(' and ')} on ${expectation.path}`, () => { + for (const role of expectation.refuse) { + expect(admitsUnitRole(expectation.path, role), `${role} was admitted`).toBe(false); + } + }); + } + + it('sends a refused caller to /unauthorised rather than just returning false', () => { + userService.currentUser.role = 'Student'; + globalState.loadedUnitRoles.currentValues = []; + + const outcome = activate( + snapshotFor({roleWhitelist: whitelistFor('units/:unitId/admin')}, UNIT_ID), + ); + + expect(outcome).toEqual({commands: ['/unauthorised']}); + }); + + it('lets a system Admin or Auditor into a unit they hold no unit role in', () => { + // roleForRoute falls back to the system role for these two. Every unit whitelist has to keep + // carrying them or site administration loses access to the units it administers. + for (const path of UNIT_ROLE_EXPECTATIONS.map((expectation) => expectation.path)) { + for (const role of ['Admin', 'Auditor']) { + globalState.loadedUnitRoles.currentValues = []; + userService.currentUser.role = role; + + expect( + activate(snapshotFor({roleWhitelist: whitelistFor(path)}, UNIT_ID)), + `${role} was refused on ${path}`, + ).toBe(true); + } + } + }); + + it('still admits a system Convenor to a unit they hold no unit role in', () => { + // Pinning a gap, not endorsing it. roleForRoute returns undefined here, but isAuthorised + // falls back to the system role when it is handed one, so the undefined never refuses + // anybody. A system Convenor with no role in unit 12 reaches /units/12/admin. The card for + // this change says not to touch the guard's logic, so this records today's behaviour and + // will go red when someone fixes it on its own ticket. + globalState.loadedUnitRoles.currentValues = []; + userService.currentUser.role = 'Convenor'; + + expect( + activate(snapshotFor({roleWhitelist: whitelistFor('units/:unitId/admin')}, UNIT_ID)), + ).toBe(true); + }); + + for (const expectation of SYSTEM_ROLE_EXPECTATIONS) { + it(`admits ${expectation.allow.join(' and ')} to ${expectation.path}`, () => { + for (const role of expectation.allow) { + expect(admitsSystemRole(expectation.path, role), `${role} was refused`).toBe(true); + } + }); + + it(`refuses ${expectation.refuse.join(' and ')} on ${expectation.path}`, () => { + for (const role of expectation.refuse) { + expect(admitsSystemRole(expectation.path, role), `${role} was admitted`).toBe(false); + } + }); + } + + it('waits for the global state to finish loading before it decides', () => { + globalState.isLoadingSubject.next(true); + userService.currentUser.role = 'Student'; + + const result = TestBed.runInInjectionContext( + () => + roleWhitelistGuard( + snapshotFor({roleWhitelist: whitelistFor('units/:unitId/admin')}, UNIT_ID), + {} as RouterStateSnapshot, + ) as Observable, + ); + + let outcome: boolean | UrlTree | undefined; + result.subscribe((value) => (outcome = value)); + expect(outcome, 'the guard decided while the roles were still loading').toBeUndefined(); + + globalState.loadedUnitRoles.currentValues = [{unit: {id: UNIT_ID}, role: 'Convenor'}]; + globalState.isLoadingSubject.next(false); + + expect(outcome).toBe(true); + }); + + describe('a guarded route with no whitelist', () => { + const noWhitelist = () => + ({ + data: {}, + paramMap: convertToParamMap({}), + parent: null, + pathFromRoot: [ + {routeConfig: {path: 'units'}}, + {routeConfig: {path: ':unitId'}}, + {routeConfig: {path: 'analytics'}}, + ], + }) as unknown as ActivatedRouteSnapshot; + + it('still lets the caller through, so a missing whitelist cannot lock production out', () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => undefined); + userService.currentUser.role = 'Student'; + + expect(activate(noWhitelist())).toBe(true); + + spy.mockRestore(); + }); + + it('logs an error naming the route so the mistake is visible while developing', () => { + const spy = vi.spyOn(console, 'error').mockImplementation(() => undefined); + + activate(noWhitelist()); + + expect(spy).toHaveBeenCalledOnce(); + expect(spy.mock.calls[0][0]).toContain('units/:unitId/analytics'); + + spy.mockRestore(); + }); + }); +}); diff --git a/src/app/common/header/header.component.html b/src/app/common/header/header.component.html index 47bef3cb70..a969dae481 100644 --- a/src/app/common/header/header.component.html +++ b/src/app/common/header/header.component.html @@ -1,19 +1,20 @@ @if (showHeader) { - @if (!media.isActive('xs')) { + @if (!isExtraSmall) { @if (logoSettings.hasLogo) { } - +
- @if (media.isActive('xs')) { + @if (isExtraSmall) { } - @if (media.isActive('xs')) { + @if (isExtraSmall) { }

Units you teach

@for (unitRole of unitRoles; track unitRole) { @if (!unitRole.unit.teachingPeriod || unitRole.unit.teachingPeriod?.active) { -
@@ -52,7 +59,7 @@

Units y

-
+ } } diff --git a/src/app/common/header/unit-dropdown/unit-dropdown.component.spec.ts b/src/app/common/header/unit-dropdown/unit-dropdown.component.spec.ts index bcf7ff444d..31cab69f47 100644 --- a/src/app/common/header/unit-dropdown/unit-dropdown.component.spec.ts +++ b/src/app/common/header/unit-dropdown/unit-dropdown.component.spec.ts @@ -1,5 +1,5 @@ -import {MediaObserver} from 'ng-flex-layout'; import {beforeEach, describe, expect, it} from 'vitest'; +import {BreakpointObserver} from '@angular/cdk/layout'; import {NO_ERRORS_SCHEMA} from '@angular/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; import {MatMenuModule} from '@angular/material/menu'; @@ -12,7 +12,7 @@ describe('UnitDropdownComponent', () => { await TestBed.configureTestingModule({ declarations: [UnitDropdownComponent], imports: [MatMenuModule], - providers: [{provide: MediaObserver, useValue: {isActive: () => false}}], + providers: [{provide: BreakpointObserver, useValue: {isMatched: () => false}}], schemas: [NO_ERRORS_SCHEMA], }).compileComponents(); }); diff --git a/src/app/common/header/unit-dropdown/unit-dropdown.component.ts b/src/app/common/header/unit-dropdown/unit-dropdown.component.ts index 4190257424..d9d7ead2f4 100644 --- a/src/app/common/header/unit-dropdown/unit-dropdown.component.ts +++ b/src/app/common/header/unit-dropdown/unit-dropdown.component.ts @@ -1,4 +1,4 @@ -import {MediaObserver} from 'ng-flex-layout'; +import {BreakpointObserver} from '@angular/cdk/layout'; import {ChangeDetectionStrategy, Component, Input} from '@angular/core'; import {Project, Unit, UnitRole} from 'src/app/api/models/doubtfire-model'; @@ -16,5 +16,9 @@ export class UnitDropdownComponent { unitTitle: string; - constructor(public media: MediaObserver) {} + get isExtraSmall(): boolean { + return this.breakpointObserver.isMatched('(max-width: 599.98px)'); + } + + constructor(private breakpointObserver: BreakpointObserver) {} } diff --git a/src/app/common/learning-outcome-editor/learning-outcome-editor.component.spec.ts b/src/app/common/learning-outcome-editor/learning-outcome-editor.component.spec.ts new file mode 100644 index 0000000000..19014647d7 --- /dev/null +++ b/src/app/common/learning-outcome-editor/learning-outcome-editor.component.spec.ts @@ -0,0 +1,41 @@ +import {describe, expect, it} from 'vitest'; +import {LearningOutcomeEditorComponent} from './learning-outcome-editor.component'; + +describe('LearningOutcomeEditorComponent.getNextOutcomeNumber', () => { + // Build the component without the Angular injector so the test stays focused + // on the numbering logic. + function editorWith(prefix: string, abbreviations: string[]): LearningOutcomeEditorComponent { + const comp = Object.create( + LearningOutcomeEditorComponent.prototype, + ) as LearningOutcomeEditorComponent; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (comp as any).abbreviationPrefix = prefix; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (comp as any).outcomeSource = {data: abbreviations.map((abbreviation) => ({abbreviation}))}; + return comp; + } + + it('returns 1 when there are no outcomes', () => { + expect(editorWith('TLO', []).getNextOutcomeNumber()).toBe(1); + }); + + it('returns max + 1, not last + 1, when rows are out of order', () => { + expect(editorWith('TLO', ['TLO1', 'TLO3', 'TLO2']).getNextOutcomeNumber()).toBe(4); + }); + + it('ignores a non-numeric abbreviation instead of producing NaN', () => { + expect(editorWith('TLO', ['TLO1', 'TLOx']).getNextOutcomeNumber()).toBe(2); + }); + + it('returns 1 when every abbreviation is non-numeric', () => { + expect(editorWith('TLO', ['TLOa', 'TLOb']).getNextOutcomeNumber()).toBe(1); + }); + + it('ignores an abbreviation where the prefix is not at the start', () => { + expect(editorWith('TLO', ['1TLO', 'TLO2']).getNextOutcomeNumber()).toBe(3); + }); + + it('ignores a decimal suffix rather than producing a fractional number', () => { + expect(editorWith('TLO', ['TLO.5', 'TLO4']).getNextOutcomeNumber()).toBe(5); + }); +}); diff --git a/src/app/common/learning-outcome-editor/learning-outcome-editor.component.ts b/src/app/common/learning-outcome-editor/learning-outcome-editor.component.ts index 183bb47c96..0bf6b4fb8d 100644 --- a/src/app/common/learning-outcome-editor/learning-outcome-editor.component.ts +++ b/src/app/common/learning-outcome-editor/learning-outcome-editor.component.ts @@ -421,11 +421,13 @@ export class LearningOutcomeEditorComponent implements OnChanges, OnInit, AfterV } getNextOutcomeNumber(): number { - if (this.outcomeSource.data && this.outcomeSource.data.length > 0) { - let abbr = this.outcomeSource.data[this.outcomeSource.data.length - 1].abbreviation; - abbr = abbr.replace(this.abbreviationPrefix, ''); - return Number(abbr) + 1; - } - return 1; + const prefix = this.abbreviationPrefix; + const numbers = (this.outcomeSource.data || []) + .map((outcome) => outcome.abbreviation ?? '') + .filter((abbreviation) => abbreviation.startsWith(prefix)) + .map((abbreviation) => abbreviation.slice(prefix.length)) + .filter((suffix) => /^\d+$/.test(suffix)) + .map((suffix) => Number(suffix)); + return numbers.length > 0 ? Math.max(...numbers) + 1 : 1; } } diff --git a/src/app/common/modals/confirmation-modal/confirmation-modal.component.html b/src/app/common/modals/confirmation-modal/confirmation-modal.component.html index beb1051f27..a682ebf91e 100644 --- a/src/app/common/modals/confirmation-modal/confirmation-modal.component.html +++ b/src/app/common/modals/confirmation-modal/confirmation-modal.component.html @@ -16,7 +16,7 @@
diff --git a/src/app/common/modals/extension-modal/extension-modal.component.spec.ts b/src/app/common/modals/extension-modal/extension-modal.component.spec.ts new file mode 100644 index 0000000000..5e3e05784f --- /dev/null +++ b/src/app/common/modals/extension-modal/extension-modal.component.spec.ts @@ -0,0 +1,21 @@ +import {describe, expect, it, vi} from 'vitest'; +import {ExtensionModalComponent} from './extension-modal.component'; + +// scrollCommentsDown reaches into the DOM for div.comments-body, which is not +// mounted on every screen the modal can open from. Exercise it on a bare +// instance with no such element present. +describe('ExtensionModalComponent scrollCommentsDown', () => { + it('does not throw when the comments panel is not on the page', () => { + vi.useFakeTimers(); + const component = Object.create(ExtensionModalComponent.prototype) as { + scrollCommentsDown(): void; + }; + + expect(() => { + component.scrollCommentsDown(); + vi.runAllTimers(); + }).not.toThrow(); + + vi.useRealTimers(); + }); +}); diff --git a/src/app/common/modals/extension-modal/extension-modal.component.ts b/src/app/common/modals/extension-modal/extension-modal.component.ts index 70ff977b6b..87f8e9f9f9 100644 --- a/src/app/common/modals/extension-modal/extension-modal.component.ts +++ b/src/app/common/modals/extension-modal/extension-modal.component.ts @@ -69,8 +69,10 @@ export class ExtensionModalComponent { private scrollCommentsDown(): void { setTimeout(() => { - const objDiv = document.querySelector('div.comments-body'); - // let wrappedResult = angular.element(objDiv); + const objDiv = document.querySelector('div.comments-body'); + if (!objDiv) { + return; + } objDiv.scrollTop = objDiv.scrollHeight; }, 50); } diff --git a/src/app/common/modals/scorm-extension-modal/scorm-extension-modal.component.ts b/src/app/common/modals/scorm-extension-modal/scorm-extension-modal.component.ts index 8322724d1b..20ffb78b20 100644 --- a/src/app/common/modals/scorm-extension-modal/scorm-extension-modal.component.ts +++ b/src/app/common/modals/scorm-extension-modal/scorm-extension-modal.component.ts @@ -41,8 +41,10 @@ export class ScormExtensionModalComponent { private scrollCommentsDown(): void { setTimeout(() => { - const objDiv = document.querySelector('div.comments-body'); - // let wrappedResult = angular.element(objDiv); + const objDiv = document.querySelector('div.comments-body'); + if (!objDiv) { + return; + } objDiv.scrollTop = objDiv.scrollHeight; }, 50); } diff --git a/src/app/common/pipes/marked.pipe.spec.ts b/src/app/common/pipes/marked.pipe.spec.ts index 5ac4060da5..0092cdb163 100644 --- a/src/app/common/pipes/marked.pipe.spec.ts +++ b/src/app/common/pipes/marked.pipe.spec.ts @@ -6,4 +6,23 @@ describe('MarkedPipe', () => { const pipe = new MarkedPipe(); expect(pipe).toBeTruthy(); }); + + it('renders a fenced code block as 
', () => {
+    const pipe = new MarkedPipe();
+    const html = pipe.transform('```\ncode\n```');
+    expect(html).toContain('
');
+  });
+
+  it('renders a markdown list as a