Repository navigation
#6624 - Search student by certificate number - #6849
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Certificate validation allows nonnumeric or overlong values, causing uncaught HTTP 400 errors that leave users without validation feedback and previous search results still displayed.
1 open issue
What changed in this PR
Adds certificate-number lookup to student search, connecting the shared search interface to exact-match disbursement queries in the API.
Changes:
- Adds a certificate-number field and converts the search component to
<script setup>. - Extends search contracts, input validation, and backend filtering.
- Adds API tests for exact matches, invalid input, and institution access restrictions.
| File | Description |
|---|---|
sources/packages/web/src/services/http/dto/Student.dto.ts |
Adds the certificate-number search parameter. |
sources/packages/web/src/components/common/SearchStudents.vue |
Adds the search field and adopts <script setup>. |
sources/packages/backend/apps/api/src/services/student/student.service.ts |
Filters students by exact disbursement certificate number. |
sources/packages/backend/apps/api/src/route-controllers/student/models/student.dto.ts |
Validates numeric certificate input up to nine digits. |
sources/packages/backend/apps/api/src/route-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts |
Tests certificate lookup with institution access restrictions. |
sources/packages/backend/apps/api/src/route-controllers/student/_tests_/e2e/student.aest.controller.searchStudents.e2e-spec.ts |
Tests exact matching, partial-match rejection, and input validation. |
Review effort: Balanced
| !input.lastName && | ||
| !input.appNumber && | ||
| !input.sin && | ||
| !input.email && | ||
| !input.documentNumber, |
There was a problem hiding this comment.
@tiago-graf Thanks for the discussion related to this. Instead of this approach, I would suggest using a class level validator to validate at least one search field must be provided.
| @IsNotEmpty() | ||
| email: string; | ||
| /** | ||
| * Certificate (financial document) number. Limited to 9 digits to fit the integer DB column. |
There was a problem hiding this comment.
From a DB perspective, it should validate up to a max int4 amount.
From a biz perspective, it can have up to 8 (full-time) or 7 (part-time).
Maybe the comment can be adjusted to
Document number (a.k.a. e-Cert Number). Limited to 9 digits to be safe, since there is no business requirement to have it restricted; currently it can have up to 8 (full-time) or 7 (part-time).
| <v-col cols="12" lg> | ||
| <v-text-field | ||
| label="Certificate number" | ||
| density="compact" | ||
| variant="outlined" | ||
| v-model.trim="documentNumber" | ||
| @keyup.enter="searchStudents" | ||
| hide-details | ||
| /> | ||
| </v-col> |
There was a problem hiding this comment.
The UI does not restrict the input, which allows more characters than the DTO allows to be submitted, causing a 400 HTTP error.
There was a problem hiding this comment.
This can be restricted to accept only digits, up to the max length allowed by the API, or use validation similar to the one used by SIN, but simpler.
There was a problem hiding this comment.
One suggestion comment: To use the class-level validator
Nice work @tiago-graf 👍
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Nice work, minor comments.
| ValidateIf, | ||
| } from "class-validator"; | ||
| import { IsValidSIN } from "../../../utilities/class-validation/custom-validators/sin-validator"; | ||
| import { IsAnyProvided } from "../../../utilities/class-validation/custom-validators/is-any-provided"; |
There was a problem hiding this comment.
Please import from the existing index.
| registerDecorator({ | ||
| name: "IsAnyProvided", | ||
| target, | ||
| // Class-level validation is not tied to any property. |
There was a problem hiding this comment.
Does the framework support class-level validation, or should this be considered a workaround?
There was a problem hiding this comment.
If the above assumption is correct, I would recommend following the framework standards unless no other way is available.
For this scenario, the validtor can be adapted to be added to every property to act similarly to a "ValidateIf".
DTO
/**
* Student search filters, of which at least one is required
* to avoid retrieving all the students.
*/
const STUDENT_SEARCH_FILTERS: Array<keyof StudentSearchAPIInDTO> = [
"firstName",
"lastName",
"appNumber",
"sin",
"email",
"documentNumber",
];export class StudentSearchAPIInDTO {
@IsOptionalIfAnyOtherProvided(STUDENT_SEARCH_FILTERS)
firstName?: string;
@IsOptionalIfAnyOtherProvided(STUDENT_SEARCH_FILTERS)
lastName?: string;
@IsOptionalIfAnyOtherProvided(STUDENT_SEARCH_FILTERS)
appNumber?: string;
@IsOptionalIfAnyOtherProvided(STUDENT_SEARCH_FILTERS)
sin?: string;
@IsOptionalIfAnyOtherProvided(STUDENT_SEARCH_FILTERS)
email?: string;
/**
* Document number (a.k.a. e-Cert Number). Limited to 9 digits to be safe, since there
* is no business requirement to have it restricted; currently it can have up to
* 8 (full-time) or 7 (part-time).
*/
@IsOptionalIfAnyOtherProvided(STUDENT_SEARCH_FILTERS)
@IsInt()
@IsPositive()
@Max(999999999)
documentNumber?: number;
}Validator DRAFT (not tested)
export function IsOptionalIfAnyOtherProvided<T extends object>(
properties: Array<keyof T & string>,
validationOptions?: ValidationOptions,
): PropertyDecorator {
return (target: object, propertyKey: string | symbol) => {
const propertyName = propertyKey as string;
ValidateIf(
(object: Record<string, unknown>) =>
!!object[propertyName] ||
!properties.some(
(property) => property !== propertyName && !!object[property],
),
)(target, propertyKey);
registerDecorator({
name: "IsOptionalIfAnyOtherProvided",
target: target.constructor,
propertyName,
options: validationOptions,
constraints: [properties],
validator: {
validate(value: unknown): boolean {
return !!value;
},
defaultMessage(args: ValidationArguments): string {
const [requiredProperties] = args.constraints as [string[]];
return `At least one of the following must be provided: ${requiredProperties.join(", ")}.`;
},
},
});
};
}There was a problem hiding this comment.
This seem to work great, i just made an adjustment to allow for a optional "properties" parameter, if not provided the validator assumes all properties in the object must be checked, this avoids the need of the list of "filter" properties names
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Thanks for making the changes. The accepted suggestion to change the DTO decorators required further discussion, please see my comments about it.
Please also check the minor Sonarcloud issues.
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Nice work, thanks for the extra effort to refactor and created the IsOptionalIfAnyOtherProvided. Looks good 👍
Summary
Adds a new field "Certificate Number" in the student search tab.