AVP feature API changes#779
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds volunteer pathway support across pathway configuration, assignment, LMS course completion, eligibility, expiry, reporting, and history status management. It introduces related API contracts, persistence fields, DTO validation, controller routes, and service workflows. ChangesVolunteer pathway lifecycle
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PathwaysController
participant PathwaysService
participant LmsClientService
participant UserPathwayHistory
Client->>PathwaysController: submit volunteer assignment
PathwaysController->>PathwaysService: validate and process request
PathwaysService->>LmsClientService: resolve active course
LmsClientService-->>PathwaysService: return courseId
PathwaysService->>UserPathwayHistory: create active history
PathwaysService-->>PathwaysController: return assignment response
PathwaysController-->>Client: return result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces volunteer-specific pathways, adding new endpoints and logic for checking eligibility, retrieving active volunteer pathways, updating history status, and handling LMS course completion webhooks. It also updates the database schema and DTOs to support volunteer configurations like term months and reapplication windows. The review feedback identifies several critical issues: expired active records bypassing the reapplication window, a concurrency race condition in the webhook transaction, incorrect error status codes on LMS dependency failures, conflicting validation decorators in the DTO, and missing organization ID validation in the controller.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pathways/pathway/pathways.service.ts (1)
784-792: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCache-hit mapping needs the new pathway fields
The cache-hit
itemsWithCountsAndTagsmapping still omitstype,allow_multiple_active,volunteer_term_months, andreapply_after_days, so list responses differ between cache miss and cache hit. Mirror those fields there as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 784 - 792, The cache-hit mapping in itemsWithCountsAndTags is missing the new pathway fields, causing different list shapes between cache miss and cache hit. Update the cache-hit object assembly in pathways.service.ts to mirror the same fields already present on the non-cache path, including type, allow_multiple_active, volunteer_term_months, and reapply_after_days, so both branches return consistent pathway items.
🧹 Nitpick comments (10)
src/pathways/pathways.module.ts (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormatting: prettier quote-style/line-wrap violations.
ESLint flags single-vs-double quote inconsistencies on the new import lines and the providers array formatting.
Also applies to: 17-17, 29-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathways.module.ts` at line 4, Fix the prettier quote-style and line-wrap violations in pathways.module.ts by aligning the new import and module metadata formatting with the project style. Update the import and the providers array inside PathwaysModule to use the same quote style as the rest of the file and wrap the providers list consistently so ESLint/prettier no longer flags the added lines.Source: Linters/SAST tools
src/pathways/common/services/lms-client.service.ts (2)
461-464: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAuth/permission failures are indistinguishable from "no data found".
validateStatus: (status) => status < 500means 4xx responses (e.g., 401/403 due to bad tenant/org headers) are treated the same as a legitimate empty result — both simply log awarnand returnnull. This could mask a misconfigured integration (e.g., wrongtenantid) as "no active batch"/"no course progress" for an extended period without raising a distinct alert. Consider logging aterrorlevel (or with a distinguishing flag) when status is in the 4xx range versus a genuine empty payload at 200.Also applies to: 487-490, 617-620, 637-640
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/common/services/lms-client.service.ts` around lines 461 - 464, The LMS client methods are treating 4xx auth/permission failures the same as an empty result, which hides misconfiguration. Update the relevant request handling in lms-client.service.ts (around the active course, batch, and course progress lookups) so 4xx responses from the axios calls are logged distinctly from genuine 200-with-empty payload cases; use a higher-severity log such as logger.error or a clear marker for 4xx while keeping the existing null return for true empty results. Apply this consistently in the affected helpers identified by the active course resolution and progress/batch lookup logic.
449-513: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormatting: multiple prettier violations flagged by ESLint.
Static analysis reports several
prettier/prettierviolations across both new methods (quote style, line wrapping for long template strings). Run the formatter to resolve.Also applies to: 604-654
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/common/services/lms-client.service.ts` around lines 449 - 513, The new LMS client methods, including getActiveCourseForPathway, contain Prettier formatting violations such as quote-style and long-line wrapping. Reformat the affected code to match the repository’s Prettier rules, paying special attention to the logger messages, axios call, and any other newly added method in this service that ESLint flags.Source: Linters/SAST tools
src/pathways/pathway/pathways.service.ts (2)
1358-1369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce parameter count / cognitive complexity of
handleVolunteerAssignment.SonarCloud flags 10 positional parameters (max 7) and cognitive complexity 18 (max 15). Consider passing a single context object (e.g.
{ userId, pathway, courseId, userGoal, created_by, updated_by, tenantId, organisationId }) and extracting the reapply‑window check into a small private helper shared withcheckVolunteerEligibility(the logic at Lines 1385‑1406 duplicates 1973‑1994).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 1358 - 1369, `handleVolunteerAssignment` has too many positional arguments and duplicated reapply-window logic, so refactor it to accept a single context object containing the existing inputs like `userId`, `pathway`, `courseId`, `userGoal`, `created_by`, `updated_by`, `tenantId`, and `organisationId`. Then extract the reapply-window eligibility check currently inside `handleVolunteerAssignment` into a small private helper and reuse that same helper from `checkVolunteerEligibility` so the shared logic lives in one place and reduces cognitive complexity.Source: Linters/SAST tools
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun Prettier — the diff has numerous formatting violations that will fail lint/CI.
ESLint (
prettier/prettier) reports formatting errors across most of the added lines (quote style, argument wrapping, trailing commas). Run the project formatter (e.g.npm run lint -- --fix/prettier --write) before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` at line 5, The diff in pathways.service.ts has multiple Prettier formatting violations that will fail lint/CI. Re-run the project formatter on the updated content (or apply the equivalent lint fix) so imports, quote style, wrapping, and trailing commas match the repo’s Prettier rules, then verify the formatting of the affected declarations in PathwaysService and related methods.Source: Linters/SAST tools
src/pathways/pathway/entities/pathway.entity.ts (1)
45-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a real Postgres enum for
typeinstead of varchar.
typeis stored asvarchar(50)with app-level validation only; a nativeenumcolumn (or a CHECK constraint) would prevent invalid values from being written outside the application layer (raw SQL, migrations, other services). Givenallow_multiple_activeis documented elsewhere as "must be true for VOLUNTEER" (seecreate-pathway.dto.ts), there's also no DB-level constraint tying these two columns together — that invariant currently relies entirely on the DTO/service layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/entities/pathway.entity.ts` around lines 45 - 56, The PathwayEntity currently stores type as a varchar and relies only on application validation, so update the PathwayEntity type column to use a native Postgres enum or an equivalent CHECK constraint, keeping PathwayType as the source of valid values. Also consider adding a database-level constraint between type and allow_multiple_active to enforce the VOLUNTEER rule referenced by create-pathway.dto.ts, so the invariant is protected outside the DTO/service layer.src/pathways/pathway/dto/update-history-status.dto.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused
IsOptionalimport.
IsOptionalis imported but never used in this file.🧹 Proposed fix
-import { IsEnum, IsNotEmpty, IsOptional, IsUUID } from "class-validator"; +import { IsEnum, IsNotEmpty, IsUUID } from "class-validator";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/dto/update-history-status.dto.ts` at line 2, Remove the unused IsOptional import from the update-history-status DTO so the import list only includes the validators that are actually referenced. Check the import statement in update-history-status.dto.ts and keep IsEnum, IsNotEmpty, and IsUUID, but drop IsOptional.Source: Linters/SAST tools
src/pathways/pathway/dto/assign-pathway.dto.ts (2)
57-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrettier formatting flagged.
Indentation on the new block (2-space) doesn't match ESLint/Prettier config expectations flagged in static analysis hints.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/dto/assign-pathway.dto.ts` around lines 57 - 65, The new course_id DTO block in AssignPathwayDto needs formatting aligned with the project’s Prettier/ESLint style. Reformat the decorators and property in assign-pathway.dto.ts so the indentation matches the existing class style and the automated formatter no longer flags the block. Use the AssignPathwayDto and course_id field as the reference point when updating the spacing and alignment.Source: Linters/SAST tools
57-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent field naming:
course_idvscourseId.This DTO uses
course_id(snake_case) whileCourseCompletionWebhookDtoandListPathwayUsersFiltersDtousecourseId(camelCase) for the same LMS course concept. Mixing conventions for the same identifier across related DTOs increases the chance of mapping mistakes in the service layer.As per path instructions, ensure similar naming conventions for variables across the codebase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/dto/assign-pathway.dto.ts` around lines 57 - 65, The AssignPathway DTO is using `course_id` while related DTOs such as `CourseCompletionWebhookDto` and `ListPathwayUsersFiltersDto` use `courseId`, creating inconsistent naming for the same concept. Update the `AssignPathwayDto` field and its associated decorators (`ApiPropertyOptional`, `Expose`, `IsUUID`, `IsOptional`) to use the camelCase identifier, and make sure any service-layer mapping or serialization code that references `course_id` is updated to the new `courseId` symbol.Source: Path instructions
src/pathways/pathway/dto/course-completion-webhook.dto.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor Prettier formatting flagged by ESLint.
Static analysis flags multi-line reformatting for these
@ApiPropertyobject literals.Also applies to: 12-12, 18-18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/dto/course-completion-webhook.dto.ts` at line 6, The `@ApiProperty` decorators in `course-completion-webhook.dto.ts` are being flagged for Prettier formatting, so update the affected `ApiProperty` object literals to match the formatter’s preferred single-line style. Check the decorator usages on the DTO fields in `CourseCompletionWebhookDto` and reformat the `description`/`format` option objects consistently so ESLint no longer reports the multi-line literal issue.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pathways/pathway/dto/active-pathway.dto.ts`:
- Around line 24-33: The Swagger metadata for active-pathway DTO should not
advertise an unsupported ALL option. Update the ApiPropertyOptional description
in active-pathway.dto and keep it aligned with PathwayType and
getActivePathway() so it only documents STANDARD and VOLUNTEER, with anything
else treated as the standard path.
In `@src/pathways/pathway/dto/create-pathway.dto.ts`:
- Around line 117-127: `CreatePathwayDto.volunteer_term_months` is incorrectly
optional for `PathwayType.VOLUNTEER`, so remove `@IsOptional()` and rely on the
existing `@ValidateIf(() => type === PathwayType.VOLUNTEER)` plus validation
decorators to require it for volunteer requests while still skipping standard
ones. Also enforce `allow_multiple_active` for volunteer pathways in the same
DTO or the corresponding service validation, since it is currently only
documented and can still be omitted even when `type` is VOLUNTEER.
In `@src/pathways/pathway/entities/user-pathway-history.entity.ts`:
- Around line 13-26: The STANDARD active-history validation in assignPathway is
currently done before the transaction, so concurrent requests can both observe
no ACTIVE record and create duplicates. Move the
currentActive/existingTargetRecord check into the same dataSource.transaction
block in PathwaysService, and add a row lock or advisory lock around the user’s
pathway-history lookup so only one STANDARD assignment can proceed at a time; if
possible, also restore a database-level partial unique constraint for STANDARD
ACTIVE histories to enforce this atomically.
In `@src/pathways/pathway/pathways.controller.ts`:
- Line 48: Remove the unused PathwayType import from the PathwaysController
module; it is not referenced by any controller method, including
getActivePathway, which uses activePathwayDto.pathwayType directly. Keep the
controller’s import list limited to symbols actually used in the class and
verify no other references to PathwayType remain in this file.
- Around line 596-607: Mirror the `assign` flow in
`PathwaysController.getActiveCourseForPathway`: validate `organisationid` with
`isUUID`, and reject missing/empty values instead of allowing `orgId` to become
an empty string. Update the `getActiveCourseForPathway` method so
`DEFAULT_ORGANISATION_ID` is only used when present and still valid, and
otherwise throw the same kind of `BadRequestException` used for tenant
validation. Keep the `pathwaysService.getActiveCourseForPathway` call unchanged
except for passing a guaranteed non-empty UUID `orgId`.
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 2186-2213: The completion retry flow is using an active-only
lookup in userPathwayHistoryRepository.findOne, so a deactivated history row can
no longer hit the notification_sent idempotency check and returns NOT_FOUND
instead of already processed. Remove the is_active: true dependency from the
duplicate-detection path, and make the notification transition atomic by using a
row lock or a conditional update around the record in pathways.service.ts so
overlapping deliveries cannot both pass the validation and send the email twice.
Keep the existing notification_sent guard, but ensure it runs against the
correct history record regardless of active state.
- Around line 1372-1383: The volunteer reassignment flow in
handleVolunteerAssignment leaves an expired ACTIVE history row untouched before
creating the replacement, so the stale assignment still appears in
ACTIVE/is_active queries. Update the existing activeRecord to EXPIRED and set
is_active=false inside the same transaction before saving the new pathway
history entry, using the userPathwayHistoryRepository path in
pathways.service.ts to keep getVolunteerActivePathways and
getActivePathway(VOLUNTEER) consistent.
---
Outside diff comments:
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 784-792: The cache-hit mapping in itemsWithCountsAndTags is
missing the new pathway fields, causing different list shapes between cache miss
and cache hit. Update the cache-hit object assembly in pathways.service.ts to
mirror the same fields already present on the non-cache path, including type,
allow_multiple_active, volunteer_term_months, and reapply_after_days, so both
branches return consistent pathway items.
---
Nitpick comments:
In `@src/pathways/common/services/lms-client.service.ts`:
- Around line 461-464: The LMS client methods are treating 4xx auth/permission
failures the same as an empty result, which hides misconfiguration. Update the
relevant request handling in lms-client.service.ts (around the active course,
batch, and course progress lookups) so 4xx responses from the axios calls are
logged distinctly from genuine 200-with-empty payload cases; use a
higher-severity log such as logger.error or a clear marker for 4xx while keeping
the existing null return for true empty results. Apply this consistently in the
affected helpers identified by the active course resolution and progress/batch
lookup logic.
- Around line 449-513: The new LMS client methods, including
getActiveCourseForPathway, contain Prettier formatting violations such as
quote-style and long-line wrapping. Reformat the affected code to match the
repository’s Prettier rules, paying special attention to the logger messages,
axios call, and any other newly added method in this service that ESLint flags.
In `@src/pathways/pathway/dto/assign-pathway.dto.ts`:
- Around line 57-65: The new course_id DTO block in AssignPathwayDto needs
formatting aligned with the project’s Prettier/ESLint style. Reformat the
decorators and property in assign-pathway.dto.ts so the indentation matches the
existing class style and the automated formatter no longer flags the block. Use
the AssignPathwayDto and course_id field as the reference point when updating
the spacing and alignment.
- Around line 57-65: The AssignPathway DTO is using `course_id` while related
DTOs such as `CourseCompletionWebhookDto` and `ListPathwayUsersFiltersDto` use
`courseId`, creating inconsistent naming for the same concept. Update the
`AssignPathwayDto` field and its associated decorators (`ApiPropertyOptional`,
`Expose`, `IsUUID`, `IsOptional`) to use the camelCase identifier, and make sure
any service-layer mapping or serialization code that references `course_id` is
updated to the new `courseId` symbol.
In `@src/pathways/pathway/dto/course-completion-webhook.dto.ts`:
- Line 6: The `@ApiProperty` decorators in `course-completion-webhook.dto.ts`
are being flagged for Prettier formatting, so update the affected `ApiProperty`
object literals to match the formatter’s preferred single-line style. Check the
decorator usages on the DTO fields in `CourseCompletionWebhookDto` and reformat
the `description`/`format` option objects consistently so ESLint no longer
reports the multi-line literal issue.
In `@src/pathways/pathway/dto/update-history-status.dto.ts`:
- Line 2: Remove the unused IsOptional import from the update-history-status DTO
so the import list only includes the validators that are actually referenced.
Check the import statement in update-history-status.dto.ts and keep IsEnum,
IsNotEmpty, and IsUUID, but drop IsOptional.
In `@src/pathways/pathway/entities/pathway.entity.ts`:
- Around line 45-56: The PathwayEntity currently stores type as a varchar and
relies only on application validation, so update the PathwayEntity type column
to use a native Postgres enum or an equivalent CHECK constraint, keeping
PathwayType as the source of valid values. Also consider adding a database-level
constraint between type and allow_multiple_active to enforce the VOLUNTEER rule
referenced by create-pathway.dto.ts, so the invariant is protected outside the
DTO/service layer.
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 1358-1369: `handleVolunteerAssignment` has too many positional
arguments and duplicated reapply-window logic, so refactor it to accept a single
context object containing the existing inputs like `userId`, `pathway`,
`courseId`, `userGoal`, `created_by`, `updated_by`, `tenantId`, and
`organisationId`. Then extract the reapply-window eligibility check currently
inside `handleVolunteerAssignment` into a small private helper and reuse that
same helper from `checkVolunteerEligibility` so the shared logic lives in one
place and reduces cognitive complexity.
- Line 5: The diff in pathways.service.ts has multiple Prettier formatting
violations that will fail lint/CI. Re-run the project formatter on the updated
content (or apply the equivalent lint fix) so imports, quote style, wrapping,
and trailing commas match the repo’s Prettier rules, then verify the formatting
of the affected declarations in PathwaysService and related methods.
In `@src/pathways/pathways.module.ts`:
- Line 4: Fix the prettier quote-style and line-wrap violations in
pathways.module.ts by aligning the new import and module metadata formatting
with the project style. Update the import and the providers array inside
PathwaysModule to use the same quote style as the rest of the file and wrap the
providers list consistently so ESLint/prettier no longer flags the added lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 489ea3b4-807d-4a85-a411-62f4c752dbc9
📒 Files selected for processing (17)
src/common/utils/api-id.config.tssrc/common/utils/response.messages.tssrc/pathways/common/services/lms-client.service.tssrc/pathways/pathway/dto/active-pathway.dto.tssrc/pathways/pathway/dto/assign-pathway.dto.tssrc/pathways/pathway/dto/check-eligibility.dto.tssrc/pathways/pathway/dto/course-completion-webhook.dto.tssrc/pathways/pathway/dto/create-pathway.dto.tssrc/pathways/pathway/dto/list-pathway-users.dto.tssrc/pathways/pathway/dto/list-pathway.dto.tssrc/pathways/pathway/dto/update-history-status.dto.tssrc/pathways/pathway/dto/update-pathway.dto.tssrc/pathways/pathway/entities/pathway.entity.tssrc/pathways/pathway/entities/user-pathway-history.entity.tssrc/pathways/pathway/pathways.controller.tssrc/pathways/pathway/pathways.service.tssrc/pathways/pathways.module.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pathways/pathway/pathways.service.ts (1)
1368-1392: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVolunteer assignment ignores
allow_multiple_activeThe volunteer path still blocks any unexpiredACTIVErecord for the same user/pathway and never checksallow_multiple_active, so that pathway setting has no effect on reassignment eligibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 1368 - 1392, The volunteer assignment eligibility check in pathways.service.ts is always blocking on any unexpired ACTIVE history record, so the allow_multiple_active setting is ignored. Update the active-record logic in the pathway assignment flow around the activeRecord lookup to first respect pathway.allow_multiple_active: only enforce the single-active conflict when that flag is false, and allow additional ACTIVE assignments when it is true. Keep the existing reapply-after expiry handling tied to activeRecord.expires_at, but make sure the early return conflict path in the eligibility block is gated by allow_multiple_active.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 1368-1392: The volunteer assignment eligibility check in
pathways.service.ts is always blocking on any unexpired ACTIVE history record,
so the allow_multiple_active setting is ignored. Update the active-record logic
in the pathway assignment flow around the activeRecord lookup to first respect
pathway.allow_multiple_active: only enforce the single-active conflict when that
flag is false, and allow additional ACTIVE assignments when it is true. Keep the
existing reapply-after expiry handling tied to activeRecord.expires_at, but make
sure the early return conflict path in the eligibility block is gated by
allow_multiple_active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2a18618-58f9-4646-90e7-92a196c0dbc2
📒 Files selected for processing (4)
src/pathways/pathway/dto/active-pathway.dto.tssrc/pathways/pathway/dto/create-pathway.dto.tssrc/pathways/pathway/pathways.controller.tssrc/pathways/pathway/pathways.service.ts
💤 Files with no reviewable changes (1)
- src/pathways/pathway/dto/create-pathway.dto.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/pathways/pathway/pathways.controller.ts (1)
728-770: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
updateHistoryStatusmissingorganisationIdUUID validation and emptyorgIdcheck.Unlike
assign(lines 529-535) which validates both UUID format and non-emptyorgId, andgetActiveCourseForPathway(lines 605-607) which at least checks for empty, this method does neither. An empty or malformedorganisationIdwould be trimmed and forwarded to the service unchecked.🛡️ Proposed fix: align with `assign` validation pattern
if (!tenantId || !isUUID(tenantId)) { throw new BadRequestException(API_RESPONSES.TENANTID_VALIDATION); } + if (organisationId && !isUUID(organisationId)) { + throw new BadRequestException(API_RESPONSES.ORGANISATIONID_VALIDATION); + } const orgId = (process.env.DEFAULT_ORGANISATION_ID || organisationId || '').trim(); + if (!orgId) { + throw new BadRequestException(API_RESPONSES.ORGANISATIONID_REQUIRED); + } return this.pathwaysService.updateHistoryStatus(id, dto, tenantId, orgId, response);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.controller.ts` around lines 728 - 770, The updateHistoryStatus handler is missing the same organisationId safeguards used elsewhere, so align it with the assign and getActiveCourseForPathway validation pattern. In pathways.controller.ts, inside updateHistoryStatus, validate organisationId for UUID format when present and ensure the derived orgId is not empty before calling pathwaysService.updateHistoryStatus. If the header/default value is invalid or blank, throw the same bad-request style response used for tenantId so malformed organisation context is never forwarded.src/pathways/pathway/dto/create-pathway.dto.ts (1)
108-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
allow_multiple_activestill not enforced for VOLUNTEER pathways.The
volunteer_term_monthsrequirement was fixed (no more@IsOptional()), butallow_multiple_activeremains@IsOptional()with no@ValidateIf/conditional check tying it totype === VOLUNTEER. The field description states it "Must be true for VOLUNTEER type," but nothing enforces this at the DTO or service layer —pathways.service.tsalso defaults it tofalseand never reads it during assignment (see comment onhandleVolunteerAssignment). This makes the field effectively decorative.🐛 Proposed validation fix
+ `@ValidateIf`((o) => o.type === PathwayType.VOLUNTEER) + `@Equals`(true, { message: "allow_multiple_active must be true for VOLUNTEER pathways" }) `@Expose`() `@IsBoolean`() `@IsOptional`() allow_multiple_active?: boolean;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/dto/create-pathway.dto.ts` around lines 108 - 116, The allow_multiple_active field in create-pathway DTO is still only optional/boolean and is not enforced for VOLUNTEER pathways. Update CreatePathwayDto so the validation for allow_multiple_active is conditional on type (using a ValidateIf-style check or equivalent) and ensure it is required/forced true when type is VOLUNTEER, then mirror that rule in pathways.service.ts where handleVolunteerAssignment applies assignment logic so the value is actually read and enforced instead of defaulting to false.src/pathways/pathway/pathways.service.ts (4)
2271-2306: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
updateHistoryStatusnever assigns volunteer tags on COMPLETED, contradicting its own docstring.The method comment states: "On COMPLETED: assigns the volunteer tag alias (volunteer_<pathway.key>) to user's auto_tags," but the transaction body only updates
status/is_active/deactivated_at/updated_by—assignVolunteerTagsis never invoked here (it's only called fromhandleCourseCompletionWebhook). If this endpoint is meant to allow manual/admin completion (not just the LMS webhook), tag assignment is missing.🐛 Proposed fix
await this.dataSource.transaction(async (manager) => { await manager.update(UserPathwayHistory, { id: historyId }, { status: dto.status, is_active: false, deactivated_at: now, updated_by: dto.updated_by, }); + + if (dto.status === PathwayHistoryStatus.COMPLETED) { + await this.assignVolunteerTags(manager, record.user_id, record.pathway); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 2271 - 2306, `updateHistoryStatus` updates the history record but never performs the COMPLETED tag assignment described in its docstring. In the `PathwaysService.updateHistoryStatus` flow, after the `manager.update(UserPathwayHistory, ...)` step, add the same volunteer-tag assignment logic used by `handleCourseCompletionWebhook` by invoking `assignVolunteerTags` when `dto.status` is COMPLETED. Make sure the logic has access to the related `record.pathway`/user data and runs within the existing transaction so manual completion also updates `auto_tags` consistently.
1998-2020: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
checkVolunteerEligibilitydoesn't validate thatdto.userIdexists.Unlike
handlePathwayAssignment,getActivePathway, andgetVolunteerActivePathways, this method skips theuserRepository.findOneexistence check. A typo'd/nonexistentuserIdsimply returnsisEligible: true(no history found), which could mislead callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 1998 - 2020, The checkVolunteerEligibility method currently skips verifying that dto.userId exists, so a missing or typoed user can incorrectly be treated as eligible. Add a userRepository.findOne existence check near the start of checkVolunteerEligibility, matching the validation flow used in handlePathwayAssignment, getActivePathway, and getVolunteerActivePathways, and return an appropriate error response if the user is not found before querying userPathwayHistoryRepository.
1309-1316: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winScope
currentActiveto STANDARD pathways.currentActivematches any active history row for the user, including VOLUNTEER assignments. Since volunteer records can remain active, a STANDARD assignment can deactivate an unrelated volunteer record in the transaction below. Filter bypathway.type = STANDARDbefore updating.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 1309 - 1316, The currentActive lookup in pathways.service.ts is too broad because it matches any active history row for the user, including volunteer assignments. Update the userPathwayHistoryRepository query in the currentActive block to scope it to STANDARD pathways by filtering through the related pathway/type field before the later deactivate/update logic runs. Keep the rest of the transaction flow intact, but ensure currentActive only represents an active STANDARD pathway record so volunteer rows are not affected.
1401-1538: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
allow_multiple_activein volunteer assignmenthandleVolunteerAssignmentstill rejects a second active record unconditionally, so VOLUNTEER pathways can never hold concurrent ACTIVE assignments even though the DTO, entity comment, and create/update flow all persistallow_multiple_active. Gate the conflict check on!pathway.allow_multiple_active, or remove the flag/docs if multiple active assignments are not meant to be supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 1401 - 1538, `handleVolunteerAssignment` is still blocking any second ACTIVE volunteer assignment even when the pathway is configured to allow multiples. Update the eligibility check in `handleVolunteerAssignment` so the existing `activeRecord` conflict response only happens when `pathway.allow_multiple_active` is false, and keep the rest of the expiry/reapply logic unchanged. Make sure the `Pathway` field is respected consistently when creating the new `UserPathwayHistory` record and when deciding whether to expire an older active record.
🧹 Nitpick comments (2)
src/pathways/pathway/dto/update-pathway.dto.ts (1)
120-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency with
CreatePathwayDto: date fields here only use@IsOptional()@IsDateString(), without the@ValidateIftype-gating added inCreatePathwayDto. Not a functional bug (service-side logic inupdate()already gates persistence byisVolunteer), but worth aligning for consistency between create/update DTOs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/dto/update-pathway.dto.ts` around lines 120 - 146, The update DTO date fields in UpdatePathwayDto are missing the same VOLUNTEER-only validation gating used in CreatePathwayDto. Align application_opening_date, application_closing_date, and notification_date with the create DTO by adding the existing type-based ValidateIf condition around the IsDateString checks so both DTOs enforce the same input rules consistently.src/pathways/pathway/pathways.service.ts (1)
1413-1461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEligibility/reapply-window logic is duplicated between
handleVolunteerAssignmentandcheckVolunteerEligibility.Both methods independently implement the "active-record-not-expired" check and the "reapply_after_days window against last COMPLETED/EXPIRED record" check (~45 lines each). Any future rule change (e.g., the
allow_multiple_activefix above) needs to be applied in two places, risking drift. Consider extracting a shared private helper, e.g.resolveVolunteerEligibility(userId, pathway): Promise<{eligible: boolean; reason?: string; reapplyAfterDate?: string}>, used by both call sites.Also applies to: 1994-2070
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 1413 - 1461, The eligibility and reapply-window checks are duplicated in both handleVolunteerAssignment and checkVolunteerEligibility, so future rule changes can drift between the two paths. Extract the shared logic into a private helper on pathways.service.ts, such as resolveVolunteerEligibility(userId, pathway), that encapsulates the active-record and reapply_after_days checks using userPathwayHistoryRepository and returns a structured result with eligibility and any rejection reason/date. Then update both handleVolunteerAssignment and checkVolunteerEligibility to call that helper instead of reimplementing the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pathways/pathway/entities/pathway.entity.ts`:
- Around line 57-65: Add a cross-field validation for the pathway date fields so
`application_closing_date` cannot be earlier than `application_opening_date`.
Update the pathway create/update validation logic that works with
`PathwayEntity` to compare both fields together, and reject the payload when the
closing date is before the opening date. Use the existing
`application_opening_date` and `application_closing_date` symbols to locate the
relevant DTO/service validation path and keep the single-field date checks in
place.
In `@src/pathways/pathway/pathways.controller.ts`:
- Around line 699-727: The courseCompletedWebhook endpoint currently only
validates the tenant id and then delegates to
pathwaysService.handleCourseCompletionWebhook, so any authenticated user could
invoke LMS completion flow if the route is exposed. Add a webhook-specific
authorization check in pathways.controller.ts for courseCompletedWebhook, such
as a shared-secret, HMAC signature, or LMS-only guard, and enforce it before
calling the service so only the LMS can trigger completion and tag assignment.
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 2139-2179: The `handleCourseCompletionWebhook` flow is not
idempotent because it only loads `ACTIVE` history rows before checking
`notification_sent`, so retried deliveries after status changes miss the record
and return not found. Update the lookup in `handleCourseCompletionWebhook` to
find the matching history regardless of final status needed for deduping, and
base the response on `notification_sent`/completion state instead of `h.status =
ACTIVE`. Also make the processing atomic by locking the row or using a
conditional update on `userPathwayHistoryRepository` before calling
`assignVolunteerTags`, so concurrent webhook deliveries cannot both proceed.
- Around line 1073-1095: The date-clearing block in pathways.service.ts is
always populating updateData for non-VOLUNTEER pathways, which prevents the
empty-update guard in update() from ever firing. In update() around the
effectiveType/isVolunteer logic, only set application_opening_date,
application_closing_date, and notification_date to null when the request is
actually changing the pathway type from VOLUNTEER to non-VOLUNTEER or when one
of those date fields is explicitly present; otherwise leave updateData untouched
so Object.keys(updateData).length can correctly detect a no-op update.
---
Outside diff comments:
In `@src/pathways/pathway/dto/create-pathway.dto.ts`:
- Around line 108-116: The allow_multiple_active field in create-pathway DTO is
still only optional/boolean and is not enforced for VOLUNTEER pathways. Update
CreatePathwayDto so the validation for allow_multiple_active is conditional on
type (using a ValidateIf-style check or equivalent) and ensure it is
required/forced true when type is VOLUNTEER, then mirror that rule in
pathways.service.ts where handleVolunteerAssignment applies assignment logic so
the value is actually read and enforced instead of defaulting to false.
In `@src/pathways/pathway/pathways.controller.ts`:
- Around line 728-770: The updateHistoryStatus handler is missing the same
organisationId safeguards used elsewhere, so align it with the assign and
getActiveCourseForPathway validation pattern. In pathways.controller.ts, inside
updateHistoryStatus, validate organisationId for UUID format when present and
ensure the derived orgId is not empty before calling
pathwaysService.updateHistoryStatus. If the header/default value is invalid or
blank, throw the same bad-request style response used for tenantId so malformed
organisation context is never forwarded.
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 2271-2306: `updateHistoryStatus` updates the history record but
never performs the COMPLETED tag assignment described in its docstring. In the
`PathwaysService.updateHistoryStatus` flow, after the
`manager.update(UserPathwayHistory, ...)` step, add the same volunteer-tag
assignment logic used by `handleCourseCompletionWebhook` by invoking
`assignVolunteerTags` when `dto.status` is COMPLETED. Make sure the logic has
access to the related `record.pathway`/user data and runs within the existing
transaction so manual completion also updates `auto_tags` consistently.
- Around line 1998-2020: The checkVolunteerEligibility method currently skips
verifying that dto.userId exists, so a missing or typoed user can incorrectly be
treated as eligible. Add a userRepository.findOne existence check near the start
of checkVolunteerEligibility, matching the validation flow used in
handlePathwayAssignment, getActivePathway, and getVolunteerActivePathways, and
return an appropriate error response if the user is not found before querying
userPathwayHistoryRepository.
- Around line 1309-1316: The currentActive lookup in pathways.service.ts is too
broad because it matches any active history row for the user, including
volunteer assignments. Update the userPathwayHistoryRepository query in the
currentActive block to scope it to STANDARD pathways by filtering through the
related pathway/type field before the later deactivate/update logic runs. Keep
the rest of the transaction flow intact, but ensure currentActive only
represents an active STANDARD pathway record so volunteer rows are not affected.
- Around line 1401-1538: `handleVolunteerAssignment` is still blocking any
second ACTIVE volunteer assignment even when the pathway is configured to allow
multiples. Update the eligibility check in `handleVolunteerAssignment` so the
existing `activeRecord` conflict response only happens when
`pathway.allow_multiple_active` is false, and keep the rest of the
expiry/reapply logic unchanged. Make sure the `Pathway` field is respected
consistently when creating the new `UserPathwayHistory` record and when deciding
whether to expire an older active record.
---
Nitpick comments:
In `@src/pathways/pathway/dto/update-pathway.dto.ts`:
- Around line 120-146: The update DTO date fields in UpdatePathwayDto are
missing the same VOLUNTEER-only validation gating used in CreatePathwayDto.
Align application_opening_date, application_closing_date, and notification_date
with the create DTO by adding the existing type-based ValidateIf condition
around the IsDateString checks so both DTOs enforce the same input rules
consistently.
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 1413-1461: The eligibility and reapply-window checks are
duplicated in both handleVolunteerAssignment and checkVolunteerEligibility, so
future rule changes can drift between the two paths. Extract the shared logic
into a private helper on pathways.service.ts, such as
resolveVolunteerEligibility(userId, pathway), that encapsulates the
active-record and reapply_after_days checks using userPathwayHistoryRepository
and returns a structured result with eligibility and any rejection reason/date.
Then update both handleVolunteerAssignment and checkVolunteerEligibility to call
that helper instead of reimplementing the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: daa52da1-24b9-425e-bb6e-67b3b69749a5
📒 Files selected for processing (5)
src/pathways/pathway/dto/create-pathway.dto.tssrc/pathways/pathway/dto/update-pathway.dto.tssrc/pathways/pathway/entities/pathway.entity.tssrc/pathways/pathway/pathways.controller.tssrc/pathways/pathway/pathways.service.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pathways/pathway/pathways.service.ts (1)
804-816: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCache-hit mapping must return the new pathway fields
list()returnstype,allow_multiple_active,volunteer_term_months,reapply_after_days, and the volunteer dates on cache miss, but the cache-hit mapper still drops them. The cached item already carries these properties, so the response shape changes depending on cache state. Add the same fields to the hit branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 804 - 816, Update the cache-hit mapper in list() to include type, allow_multiple_active, volunteer_term_months, reapply_after_days, application_opening_date, application_closing_date, and notification_date from the cached item, matching the cache-miss response shape and preserving null defaults where applicable.
🧹 Nitpick comments (1)
src/pathways/pathway/pathways.service.ts (1)
2305-2318: 🩺 Stability & Availability | 🔵 TrivialAutomatic volunteer expiry is disabled.
The
@Cron('0 2 * * *')decorator is commented out, socheckAndExpireVolunteerPathwaysnever runs on a schedule andCOMPLETED → EXPIREDtransitions only occur via the manual trigger endpoint. Eligibility/reapply checks compute againstexpires_at/deactivated_atdirectly, so behavior is not broken, but any reporting or filtering byEXPIREDstatus will be stale until someone hits the manual endpoint. If this is intentional for now, consider an external scheduler invokingPOST cron/expire-volunteer-pathways; otherwise re-enable the decorator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/pathway/pathways.service.ts` around lines 2305 - 2318, Automatic volunteer pathway expiry is disabled because checkAndExpireVolunteerPathways has its `@Cron` decorator commented out; re-enable `@Cron`('0 2 * * *') so the method runs daily, or document and configure an equivalent external scheduler invoking the manual expiry endpoint if disabling it is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 804-816: Update the cache-hit mapper in list() to include type,
allow_multiple_active, volunteer_term_months, reapply_after_days,
application_opening_date, application_closing_date, and notification_date from
the cached item, matching the cache-miss response shape and preserving null
defaults where applicable.
---
Nitpick comments:
In `@src/pathways/pathway/pathways.service.ts`:
- Around line 2305-2318: Automatic volunteer pathway expiry is disabled because
checkAndExpireVolunteerPathways has its `@Cron` decorator commented out; re-enable
`@Cron`('0 2 * * *') so the method runs daily, or document and configure an
equivalent external scheduler invoking the manual expiry endpoint if disabling
it is intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4868c404-6385-4737-8b55-bf21f3f52441
📒 Files selected for processing (6)
src/common/utils/api-id.config.tssrc/pathways/pathway/dto/course-completion-webhook.dto.tssrc/pathways/pathway/dto/update-history-status.dto.tssrc/pathways/pathway/entities/user-pathway-history.entity.tssrc/pathways/pathway/pathways.controller.tssrc/pathways/pathway/pathways.service.ts
💤 Files with no reviewable changes (2)
- src/pathways/pathway/dto/course-completion-webhook.dto.ts
- src/pathways/pathway/entities/user-pathway-history.entity.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/utils/api-id.config.ts
- src/pathways/pathway/dto/update-history-status.dto.ts
|
1 participant
Summary by CodeRabbit