Conversation
- Introduced `AssessmentReminderCard` component to manage evaluation reminder templates. - Integrated reminder card into the `SettingsPage`. - Implemented functionality to save reminder templates and send evaluation reminders for self, peer, and tutor evaluations. - Added backend support for fetching incomplete reminder recipients and sending reminder emails. - Created new DTOs and SQL queries for managing evaluation reminder data. - Enhanced mailing service to handle evaluation reminder sending with detailed reports on success and failure.
- Implemented SendManualMailToParticipants function to send custom emails to specified recipients. - Created new DTOs for manual mail requests and reports. - Added SQL scripts for testing manual mail functionality. - Removed deprecated evaluation reminder functionality. - Updated mailing router to handle manual mail requests. - Added unit tests for manual mail sending, including success and error scenarios.
2 tasks
Contributor
|
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:
📝 WalkthroughWalkthroughAdds an evaluation reminder feature: frontend UI and network call, assessment service endpoints and DTOs for computing recipients and triggering sends, core mailing endpoint and mailer implementation, DB schema/queries, and tests across assessment and core services. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Browser
participant Frontend as Assessment UI
participant AssessmentAPI as Assessment Service
participant CoreAPI as Core Service
participant DB as Database
User->>Frontend: Open Settings → view reminder card
Frontend->>AssessmentAPI: GET /course_phase/{id}/config/reminders/incomplete?type=...
AssessmentAPI->>DB: Query course_phase_config & evaluation_completion
DB-->>AssessmentAPI: Incomplete participant IDs & metadata
AssessmentAPI-->>Frontend: EvaluationReminderRecipients
User->>Frontend: Confirm "Send Reminder"
Frontend->>AssessmentAPI: POST /course_phase/{id}/config/reminders/send (type=...)
AssessmentAPI->>DB: Resolve recipients & template
AssessmentAPI->>CoreAPI: POST /mailing/{coursePhaseID}/manual (subject, content, recipient IDs)
CoreAPI->>DB: Get participant contact info
DB-->>CoreAPI: Participant emails
CoreAPI->>CoreAPI: Render templates & send emails
CoreAPI->>DB: Update lastSentAt in course_phase.restricted_data
CoreAPI-->>AssessmentAPI: ManualMailReport
AssessmentAPI->>DB: Persist lastSentAtByType
AssessmentAPI-->>Frontend: EvaluationReminderSendReport
Frontend->>User: Show send report (success/fail)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx`:
- Around line 192-212: handleSaveTemplate is calling updateCoursePhase with only
id and restrictedData, which causes the backend to receive missing name and
studentReadableData and overwrite them; fix by including the full current course
phase fields in the payload: read coursePhase.name and
coursePhase.studentReadableData (or fetch the full CoursePhase if coursePhase is
partial) and pass them along in the updateCoursePhase call along with id and
restrictedData, ensuring you send name, studentReadableData, and restrictedData
(mailingSettings.assessmentReminder) so the UpdateCoursePhase DTO is fully
populated; alternatively, if you prefer a backend change, refactor
updateCoursePhase to support true partial updates, but for this frontend fix
updateCoursePhase from handleSaveTemplate to include coursePhase.name and
coursePhase.studentReadableData.
In
`@clients/assessment_component/src/assessment/pages/SettingsPage/SettingsPage.tsx`:
- Line 104: The SettingsPage renders AssessmentReminderCard unconditionally;
gate it with the same permission check used for other admin features by wrapping
the AssessmentReminderCard in the existing isPromptAdmin || isLecturer
conditional (the boolean/props used in SettingsPage). Locate the
AssessmentReminderCard usage in SettingsPage (component SettingsPage) and only
render it when isPromptAdmin || isLecturer evaluates to true so
non-admin/lecturer users cannot access reminder-template editing or send-email
actions.
In `@servers/assessment/coursePhaseConfig/evaluationReminderSend.go`:
- Around line 113-128: The updateCoreCoursePhaseFn call can fail after emails
were sent, so enhance handling by logging the failure as a warning (including
the error and coursePhase.ID/evaluationType) while still returning the populated
report and the error; specifically, around the updateCoreCoursePhaseFn
invocation in evaluationReminderSend.go, keep the existing setLastSentAt(...)
and the return of report, err, but add a warning log (using the module's logger)
when updateCoreCoursePhaseFn returns an error so callers can see that mails
succeeded but persistence failed.
In `@servers/core/db/sqlc/mailing.sql.go`:
- Around line 231-298: GetParticipantMailingInformationByIDsRow.Email is a
nullable pgtype.Text but the mailing code calls sendMailFn with
participant.Email.String without checking validity; fix by checking
participant.Email.Valid before using participant.Email.String in the participant
mailing loop (where sendMailFn is invoked in manualMail.go) and skip or log
participants with invalid/NULL emails instead of calling sendMailFn with an
empty address so no malformed sends or bad logs occur.
In `@servers/core/mailing/manualMail_test.go`:
- Around line 91-140: The test TestSendManualMailHappyPath can panic when
accessing sentMails[0]/[1] because assert.Len is non-fatal; change the length
assertion to a fatal check (use suite.Require().Len(sentMails, 2)) before
accessing sentMails, or add a guard that returns/fails if len(sentMails) < 2;
update the assertion in this test (and any similar checks) so sentMails is
verified with suite.Require() prior to indexing after calling
SendManualMailToParticipants.
In `@servers/core/mailing/router.go`:
- Around line 84-95: The handler currently maps all errors from sendManualMailFn
to HTTP 500; change it to distinguish validation errors from internal errors by
checking the error returned by sendManualMailFn/SendManualMailToParticipants
(use errors.Is(err, ErrValidation) if you add a sentinel ErrValidation, or
type-assert/assert interface for a ValidationError struct returned by
SendManualMailToParticipants). If the error is a validation error, call
handleError(c, http.StatusBadRequest, err); otherwise keep the
http.StatusInternalServerError path; update SendManualMailToParticipants to
return the sentinel or typed validation error if not already.
🧹 Nitpick comments (9)
clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx (1)
141-148: Consider narrowing theerror: anytype.Using
anymakes it easy to miss actual error shapes. If the project uses axios, typing asAxiosError(or a custom error type) would be more robust. This is a minor readability improvement.servers/assessment/coursePhaseConfig/reminderRecipients.go (1)
226-237: Minor:deduplicateUUIDsduplicates logic frommailing.deduplicateUUIDList.Both
deduplicateUUIDs(here) anddeduplicateUUIDListinservers/core/mailing/manualMail.goimplement identical UUID deduplication. They're in different services (assessment vs core), so sharing isn't straightforward. Noting for awareness — not actionable now.servers/assessment/docs/docs.go (1)
5190-5223: Role-based access control is properly enforced; consider returning anonymized feedback instead of email addresses.The
/reminders/sendendpoint correctly restricts access toPromptAdminandCourseLecturerroles viaauthMiddleware, so unauthorized users cannot access these email addresses. However, for a reminder-sending endpoint, returning the list of successful/failed email addresses may be unnecessary—consider whether sending counts or anonymized identifiers would suffice for admin feedback while reducing PII surface area.servers/core/docs/swagger.json (2)
2660-2712: New manual mailing endpoint looks consistent with existing patterns.The endpoint structure, parameters, and response codes follow the conventions established by the existing
PUT /mailing/{coursePhaseID}endpoint. No structural issues.One minor note: neither this endpoint nor the existing mailing endpoint documents
401/403responses, even though these are privileged operations. If auth middleware handles these before reaching the handler, this is fine, but it may be worth documenting for API consumers.
4326-4371: Consider addingformat: "date-time"tosentAtand documenting required fields.The
sentAtfield (Line 4338) is typed as plainstring. Adding"format": "date-time"would help API consumers and code generators produce correct types. Similarly, none of the properties onSendManualMailRequestare marked as required in the spec —subject,content, andrecipientCourseParticipationIDslikely should be. If validation is enforced at the handler level this isn't a bug, but it weakens the API contract documentation.servers/core/mailing/manualMail.go (2)
56-84: Consider adding a warning log for individual send failures.When
sendMailFnreturns an error (line 79), the failure is silently added toreport.FailedEmailswithout any log output. Adding alog.Warn(orlog.WithError(err).Warn) would help operators diagnose delivery issues in production without needing to inspect the HTTP response.As per coding guidelines, "Use sirupsen/logrus for logging (Debug, Info, Warn, Error levels) in Go services".
Proposed logging addition
+ log "github.com/sirupsen/logrus"if err := sendMailFn(courseMailingSettings, participant.Email.String, finalSubject, finalContent); err != nil { + log.WithError(err).WithField("email", participant.Email.String).Warn("Failed to send manual mail to participant") report.FailedEmails = append(report.FailedEmails, participant.Email.String) continue }
72-74: Additional placeholders can override built-in participant placeholders.The loop at lines 72-74 merges
request.AdditionalPlaceholdersinto the same map that already contains built-in keys likefirstName,lastName,servers/assessment/coursePhaseConfig/evaluationReminderSend.go (1)
131-172: HTTP clients are created per-call; consider reusing a shared client.Each of
getCoreCoursePhase,sendManualReminderMail, andupdateCoreCoursePhaseinstantiates a newhttp.Client{Timeout: ...}. For a manual trigger that runs infrequently this is fine, but a shared package-level client would enable connection reuse and reduce allocation overhead if call frequency increases.Example: shared client
+var coreHTTPClient = &http.Client{Timeout: 20 * time.Second} + func getCoreCoursePhase(ctx context.Context, authHeader string, coursePhaseID uuid.UUID) (coreCoursePhaseResponse, error) { // ... - client := &http.Client{Timeout: 15 * time.Second} - resp, err := client.Do(req) + resp, err := coreHTTPClient.Do(req)Also applies to: 174-219, 221-261
servers/assessment/docs/swagger.yaml (1)
2042-2046: Consider adding enum constraint for thetypequery parameter.The
typequery parameter is documented as a plainstring, but it should only acceptself,peer, ortutor. Adding anEnumsannotation in the Go source (e.g.,//@paramtype query string true "Evaluation type" Enums(self, peer, tutor)) would produce a more self-documenting API spec and enable client-side validation.
…minders-to-assessors-once-assessment-deadline-has-passed # Conflicts: # .gitignore # servers/assessment/coursePhaseConfig/router.go # servers/assessment/coursePhaseConfig/router_test.go
…ce-assessment-deadline-has-passed
- Conditionally render AssessmentReminderCard for admins and lecturers - Improve error handling in SendEvaluationReminderManualTrigger function - Add logging for manual mail validation errors and recipient email checks - Update tests to validate error handling for manual mail sending
…nt-deadline-has-passed' of github.com:prompt-edu/prompt into 618-send-automatic-reminders-to-assessors-once-assessment-deadline-has-passed
…nformation handling
…ce-assessment-deadline-has-passed
…minders-to-assessors-once-assessment-deadline-has-passed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx (2)
49-54: Preferinterfaceovertypefor object structures.Per coding guidelines, use
interfacefor defining object structures. This also enables declaration merging if needed later.♻️ Suggested change
-type ReminderTypeConfig = { +interface ReminderTypeConfig { type: EvaluationReminderType label: string enabled: boolean deadline?: Date }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx` around lines 49 - 54, Replace the type alias ReminderTypeConfig with an equivalent interface to follow project conventions; define interface ReminderTypeConfig { type: EvaluationReminderType; label: string; enabled: boolean; deadline?: Date } so the shape stays identical but enables declaration merging later and matches the guideline referenced in AssessmentReminderCard.tsx where ReminderTypeConfig is used.
374-378: Consider using the friendly label in the confirmation dialog.The dialog displays the raw evaluation type (
self,peer,tutor) instead of the user-friendly label (Self Evaluation, etc.) defined inreminderTypes.♻️ Suggested improvement
+ {confirmationType && (() => { + const config = reminderTypes.find(r => r.type === confirmationType) + return `Send reminder emails for ${config?.label ?? confirmationType} to all currently incomplete students?` + })()} - {confirmationType && - `Send reminder emails for ${confirmationType} evaluations to all currently incomplete students?`}Alternatively, extract the label lookup into a helper:
const getConfirmationLabel = () => { if (!confirmationType) return '' const config = reminderTypes.find(r => r.type === confirmationType) return config?.label ?? confirmationType }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx` around lines 374 - 378, The confirmation dialog currently interpolates the raw confirmationType value; update it to display the user-friendly label from reminderTypes instead. In AssessmentReminderCard (where confirmationType is used in the DialogDescription), look up the matching reminderTypes entry (e.g., find by r.type === confirmationType) and use its .label (falling back to confirmationType if not found). You can inline the lookup or add a small helper like getConfirmationLabel() and reference that in the DialogDescription so the dialog shows "Self Evaluation"/"Peer Evaluation"/etc. instead of "self"/"peer"/"tutor".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx`:
- Around line 208-218: The updateCoursePhase call is overwriting
coursePhase.restrictedData by supplying only mailingSettings and thus deleting
other fields; update the payload to spread the existing restrictedData and merge
mailingSettings so other restricted fields are preserved (e.g., include
...coursePhase.restrictedData and for mailingSettings merge existing mailing
settings with the new assessmentReminder), referencing updateCoursePhase,
coursePhase, restrictedData, mailingSettings, and updatedReminder to locate the
change.
---
Nitpick comments:
In
`@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx`:
- Around line 49-54: Replace the type alias ReminderTypeConfig with an
equivalent interface to follow project conventions; define interface
ReminderTypeConfig { type: EvaluationReminderType; label: string; enabled:
boolean; deadline?: Date } so the shape stays identical but enables declaration
merging later and matches the guideline referenced in AssessmentReminderCard.tsx
where ReminderTypeConfig is used.
- Around line 374-378: The confirmation dialog currently interpolates the raw
confirmationType value; update it to display the user-friendly label from
reminderTypes instead. In AssessmentReminderCard (where confirmationType is used
in the DialogDescription), look up the matching reminderTypes entry (e.g., find
by r.type === confirmationType) and use its .label (falling back to
confirmationType if not found). You can inline the lookup or add a small helper
like getConfirmationLabel() and reference that in the DialogDescription so the
dialog shows "Self Evaluation"/"Peer Evaluation"/etc. instead of
"self"/"peer"/"tutor".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00fcde7d-9d74-48be-95cc-2ec31035eada
📒 Files selected for processing (3)
.gitignoreclients/assessment_component/src/assessment/pages/SettingsPage/SettingsPage.tsxclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- clients/assessment_component/src/assessment/pages/SettingsPage/SettingsPage.tsx
… in AssessmentReminderCard
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx (1)
49-54: Consider usinginterfaceinstead oftypefor object structures.Per coding guidelines, prefer
interfaceovertypefor defining object structures in TypeScript.♻️ Suggested change
-type ReminderTypeConfig = { +interface ReminderTypeConfig { type: EvaluationReminderType label: string enabled: boolean deadline?: Date }As per coding guidelines: "Prefer interface over type for defining object structures in TypeScript"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx` around lines 49 - 54, The declaration ReminderTypeConfig is currently a TypeScript type alias for an object shape; per project guidelines replace it with an interface declaration (interface ReminderTypeConfig) preserving the same members (type: EvaluationReminderType, label: string, enabled: boolean, deadline?: Date) so other usages of ReminderTypeConfig continue to work; update any imports/exports if necessary to reflect the interface export.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx`:
- Around line 49-54: The declaration ReminderTypeConfig is currently a
TypeScript type alias for an object shape; per project guidelines replace it
with an interface declaration (interface ReminderTypeConfig) preserving the same
members (type: EvaluationReminderType, label: string, enabled: boolean,
deadline?: Date) so other usages of ReminderTypeConfig continue to work; update
any imports/exports if necessary to reflect the interface export.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86610ed8-a0d1-4a68-a7ea-b1d91baa50c0
📒 Files selected for processing (1)
clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx
…sessmentReminderCard
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/utils.ts (1)
136-195: Please add unit coverage aroundgetReminderTypes.This helper now owns most of the client-side eligibility rules, and edge cases like single-member teams, no tutors, and partially completed peer reviews are easy to regress. A small table-driven test matrix here would make the confirmation counts much safer to change later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx`:
- Around line 196-203: getDisableReason currently allows opening confirmation
while recipient counts are still loading; update getDisableReason in
AssessmentReminderCard to return a disable reason when
isEvaluationCompletionsPending is true (e.g., "Recipient counts are still
loading; try again shortly.") so the UI won't confirm against inflated counts;
reference the isEvaluationCompletionsPending flag and ensure this check occurs
before checks that assume evaluationCompletions (for example before the
deadline/recipient checks and consistent with how getReminderTypes uses
evaluationCompletions).
- Around line 126-132: The effect currently overwrites subject/content on every
coursePhase refetch; change it so parseReminderMetaData and the setters
(setInitialMetaData, setSubject, setContent) only run on initial load or when
the phase identity changes: include phaseId in the dependency list (or track a
mounted/initialized phase id with a useRef) and, inside the useEffect, return
early unless coursePhase exists and either initial meta is not yet set or the
current phaseId differs from the last-initialized phaseId; call
parseReminderMetaData(coursePhase) and setInitialMetaData/setSubject/setContent
only in that case so unsaved edits are not clobbered by refetches.
In
`@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/components/ReminderSendConfirmationDialog.tsx`:
- Around line 36-47: The dialog currently renders two DialogDescription
components (in ReminderSendConfirmationDialog) which produce duplicate ids;
change the JSX to render a single DialogDescription that conditionally composes
both messages: the primary confirmation string built from
confirmationReminderType (using confirmationReminderType.label and
recipientCount) and, if previousSentAt exists, append the "already sent on
{formatSentAt(previousSentAt)}..." sentence (separated by punctuation or a block
element) so only one DialogDescription is present for accessibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78af6f49-a206-4f58-bc41-f4ead210b8d1
📒 Files selected for processing (6)
clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsxclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/components/ManualReminderSendingSection.tsxclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/components/ReminderSendConfirmationDialog.tsxclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/components/ReminderTemplateEditor.tsxclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/interfaces/ReminderTypeConfig.tsclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/utils.ts
✅ Files skipped from review due to trivial changes (1)
- clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/interfaces/ReminderTypeConfig.ts
…ce-assessment-deadline-has-passed
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/utils.test.ts (3)
33-40: Consider using a test framework instead of custom assertions.The custom
assertDeepEqualusingJSON.stringifyworks for the current tests but has limitations: no meaningful diff output on failure, order-sensitivity for object keys, and doesn't handle edge cases likeundefinedproperties. A test framework like Vitest (commonly used with React/TypeScript projects) would provide better assertion utilities, test isolation, and CI reporting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/utils.test.ts` around lines 33 - 40, Replace the custom assertDeepEqual helper in utils.test.ts with your test framework's assertion utilities: remove the assertDeepEqual function and replace its usages with Vitest/Jest style assertions (e.g., use expect(actual).toEqual(expected) or expect(actual).toStrictEqual(expected)); import the framework's expect where needed and update tests to rely on the framework for deep equality so you get better diffs, stable key-order handling, and edge-case coverage instead of JSON.stringify-based comparisons.
176-178: Tests execute at module load without framework integration.While this approach works for basic validation, it lacks test isolation, descriptive failure output, and CI reporting integration. Also, self-evaluation scenarios are not covered.
Consider migrating to Vitest (already common in React/TypeScript projects) for better tooling:
♻️ Example migration to Vitest
import { describe, expect, it } from 'vitest' describe('getReminderTypes', () => { it('returns zero recipients for single-member peer team', () => { const reminderTypes = getReminderTypes(/* ... */) expect(reminderTypes).toEqual([/* ... */]) }) it('returns zero recipients when team has no tutors', () => { // ... }) it('counts incomplete peer evaluations correctly', () => { // ... }) it('handles self evaluation type', () => { // Add missing coverage }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/utils.test.ts` around lines 176 - 178, The current tests run at module load via bare calls to testSingleMemberPeerTeam(), testNoTutorRecipients(), and testPartiallyCompletedPeerEvaluations(), which bypasses a test runner and lack isolation and CI reporting; refactor these into a Vitest suite by importing describe/it/expect and wrapping assertions for getReminderTypes (and any helper functions) into descriptive it blocks, add a new it case to cover the missing self-evaluation scenario, and replace the top-level function calls with expect-based assertions inside the it blocks so tests run under Vitest and produce proper failure output and reporting.
42-61: Type assertion bypasses interface validation.The
as unknown as AssessmentParticipationWithStudentcast prevents TypeScript from validating that the fixture matches the interface. IfAssessmentParticipationWithStudentadds required fields later, these tests would still compile but may produce incorrect results.Consider using a partial type approach or satisfies operator:
♻️ Alternative using Partial type
const buildParticipation = ( courseParticipationID: string, teamID: string, -): AssessmentParticipationWithStudent => - ({ +): Partial<AssessmentParticipationWithStudent> => ({ coursePhaseID: 'phase-1', courseParticipationID, passStatus: 'NOT_ASSESSED', restrictedData: {}, studentReadableData: {}, prevData: {}, student: { id: courseParticipationID, firstName: 'Test', lastName: 'Student', email: `${courseParticipationID}@example.com`, hasUniversityAccount: true, }, teamID, - }) as unknown as AssessmentParticipationWithStudent + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/utils.test.ts` around lines 42 - 61, The test fixture uses a double type cast "as unknown as AssessmentParticipationWithStudent" which bypasses compile-time validation; update buildParticipation to return a safer typed fixture by either signing its literal with the satisfies operator (e.g. the object literal ... satisfies AssessmentParticipationWithStudent) or change the function's return type to Partial<AssessmentParticipationWithStudent> (or Partial with required overrides) so TypeScript will validate fields; locate the buildParticipation function and replace the double-cast with one of these approaches and adjust the fixture fields if the compiler reports missing properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/utils.test.ts`:
- Around line 33-40: Replace the custom assertDeepEqual helper in utils.test.ts
with your test framework's assertion utilities: remove the assertDeepEqual
function and replace its usages with Vitest/Jest style assertions (e.g., use
expect(actual).toEqual(expected) or expect(actual).toStrictEqual(expected));
import the framework's expect where needed and update tests to rely on the
framework for deep equality so you get better diffs, stable key-order handling,
and edge-case coverage instead of JSON.stringify-based comparisons.
- Around line 176-178: The current tests run at module load via bare calls to
testSingleMemberPeerTeam(), testNoTutorRecipients(), and
testPartiallyCompletedPeerEvaluations(), which bypasses a test runner and lack
isolation and CI reporting; refactor these into a Vitest suite by importing
describe/it/expect and wrapping assertions for getReminderTypes (and any helper
functions) into descriptive it blocks, add a new it case to cover the missing
self-evaluation scenario, and replace the top-level function calls with
expect-based assertions inside the it blocks so tests run under Vitest and
produce proper failure output and reporting.
- Around line 42-61: The test fixture uses a double type cast "as unknown as
AssessmentParticipationWithStudent" which bypasses compile-time validation;
update buildParticipation to return a safer typed fixture by either signing its
literal with the satisfies operator (e.g. the object literal ... satisfies
AssessmentParticipationWithStudent) or change the function's return type to
Partial<AssessmentParticipationWithStudent> (or Partial with required overrides)
so TypeScript will validate fields; locate the buildParticipation function and
replace the double-cast with one of these approaches and adjust the fixture
fields if the compiler reports missing properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74196505-b9ac-4ab0-8c4a-89964dfef4c8
📒 Files selected for processing (4)
clients/assessment_component/package.jsonclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsxclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/components/ReminderSendConfirmationDialog.tsxclients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/utils.test.ts
✅ Files skipped from review due to trivial changes (1)
- clients/assessment_component/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard/components/ReminderSendConfirmationDialog.tsx
- clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentReminderCard.tsx
…minders-to-assessors-once-assessment-deadline-has-passed
- Refactor AssessmentReminderCard for improved layout and accessibility. - Update ManualReminderSendingSection and ReminderTemplateEditor for better UI consistency. - Adjust evaluation reminder sending logic to handle deadline checks more robustly. - Implement tests for evaluation reminder functionality, ensuring correct behavior on deadline boundaries. - Improve error handling in manual mail sending process.
…ce-assessment-deadline-has-passed
…minders-to-assessors-once-assessment-deadline-has-passed
…ce-assessment-deadline-has-passed
…ce-assessment-deadline-has-passed
…ce-assessment-deadline-has-passed
…ce-assessment-deadline-has-passed
…ce-assessment-deadline-has-passed
…ce-assessment-deadline-has-passed
…minders-to-assessors-once-assessment-deadline-has-passed
…r reminder template validation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Pull Request Template
✨ What is the change?
This PR adds lecturer-triggered assessment reminder mails for incomplete evaluations after the configured deadline has passed.
Evaluation Reminder Mailingcard to the assessment settings page where lecturers can save a shared reminder subject/body, review per-evaluation deadlines, and trigger reminder sends for self, peer, or tutor evaluations.📌 Reason for the change / Link to issue
Implements #618.
Assessment phases need a way to follow up with assessors who still have incomplete self, peer, or tutor evaluations once the configured deadline has passed. This branch adds the missing recipient calculation and mailing flow so lecturers can send targeted reminders instead of handling follow-up manually.
🧪 How to Test
Evaluation Reminder Mailing, making sure course mailing is configured as well.go test ./coursePhaseConfiginservers/assessment.go test ./mailinginservers/core.🖼️ Screenshots (if UI changes are included)
N/A - the branch includes a small settings-page UI addition, but no screenshots were captured while preparing this PR description.
✅ PR Checklist
Summary by CodeRabbit
New Features
Documentation
Chore